[PATCH v2] Sanitize access to gdbarch on the probe API (and fix ARM bug)

[PATCH v2] Sanitize access to gdbarch on the probe API (and fix ARM bug)

[Sergio Durigan Junior]

This is the second attempt of this patch.  After Tom's cleanup, the
patch became much clearer!  Thanks, Tom!

The first message on the thread is:

<https://sourceware.org/ml/gdb-patches/2013-12/msg00173.html>

Well, basically the idea is to sanitize the way the probe API (and
SystemTap's API) access gdbarch.  Currently, as explained in the first
attempt, the gdbarch being used comes from the objfile, which causes a
bug on ARM systems because the SDT code needs to know the target's
registers and such, and with the objfile's gdbarch on ARM the knowledge
about pseudo-registers is not present.  Therefore, after discussions
with Pedro, the decision was to use the selected frame's gdbarch.  This
patch does that.

The code has been tested on x86_64 Fedora 17 and ARM Fedora 19.

-- 
Sergio

2013-12-06  Sergio Durigan Junior  <sergiodj@redhat.com>

	* probe.c (get_probe_argument_count, can_evaluate_probe_arguments)
	(evaluate_probe_argument): Pass selected frame to the
	corresponding probe_ops method being called.
	* probe.h (struct probe_ops) <get_probe_argument_count,
	can_evaluate_probe_arguments, evaluate_probe_argument>: Adjust
	declarations to accept frame as argument.
	* stap-probe.c (stap_parse_probe_arguments): Adjust declaration to
	accept gdbarch as argument.  Remove call to get_objfile_arch.
	(stap_get_probe_argument_count): Adjust declaration to accept
	frame as argument.  Obtain gdbarch from frame.  Call
	can_evaluate_probe_arguments directly instead of the corresponding
	probe_ops method.  Provide gdbarch to stap_parse_probe_arguments.
	(stap_get_arg): Adjust declaration to accept gdbarch as argument.
	Pass it to stap_parse_probe_arguments.
	(stap_can_evaluate_probe_arguments): Adjust declaration to accept
	frame as argument.  Obtain gdbarch from it.
	(stap_evaluate_probe_argument): Likewise.  Pass gdbarch to
	stap_get_arg.
	(stap_compile_to_ax): Use gdbarch from agent_expr.
	(compute_probe_arg): Get gdbarch from frame.

diff --git a/gdb/probe.c b/gdb/probe.c
index c1e0111..b611282 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -616,7 +616,9 @@ info_probes_command (char *arg, int from_tty)
 unsigned
 get_probe_argument_count (struct probe *probe)
 {
-  return probe->pops->get_probe_argument_count (probe);
+  struct frame_info *frame = get_selected_frame (_("No frame selected"));
+
+  return probe->pops->get_probe_argument_count (probe, frame);
 }
 
 /* See comments in probe.h.  */
@@ -624,7 +626,9 @@ get_probe_argument_count (struct probe *probe)
 int
 can_evaluate_probe_arguments (struct probe *probe)
 {
-  return probe->pops->can_evaluate_probe_arguments (probe);
+  struct frame_info *frame = get_selected_frame (_("No frame selected"));
+
+  return probe->pops->can_evaluate_probe_arguments (probe, frame);
 }
 
 /* See comments in probe.h.  */
@@ -632,7 +636,9 @@ can_evaluate_probe_arguments (struct probe *probe)
 struct value *
 evaluate_probe_argument (struct probe *probe, unsigned n)
 {
-  return probe->pops->evaluate_probe_argument (probe, n);
+  struct frame_info *frame = get_selected_frame (_("No frame selected"));
+
+  return probe->pops->evaluate_probe_argument (probe, n, frame);
 }
 
 /* See comments in probe.h.  */
diff --git a/gdb/probe.h b/gdb/probe.h
index dd5387b..6184f6a 100644
--- a/gdb/probe.h
+++ b/gdb/probe.h
@@ -71,19 +71,22 @@ struct probe_ops
 
     /* Return the number of arguments of PROBE.  */
 
-    unsigned (*get_probe_argument_count) (struct probe *probe);
+    unsigned (*get_probe_argument_count) (struct probe *probe,
+					  struct frame_info *frame);
 
     /* Return 1 if the probe interface can evaluate the arguments of probe
        PROBE, zero otherwise.  See the comments on
        sym_probe_fns:can_evaluate_probe_arguments for more details.  */
 
-    int (*can_evaluate_probe_arguments) (struct probe *probe);
+    int (*can_evaluate_probe_arguments) (struct probe *probe,
+					 struct frame_info *frame);
 
     /* Evaluate the Nth argument from the PROBE, returning a value
        corresponding to it.  The argument number is represented N.  */
 
     struct value *(*evaluate_probe_argument) (struct probe *probe,
-					      unsigned n);
+					      unsigned n,
+					      struct frame_info *frame);
 
     /* Compile the Nth argument of the PROBE to an agent expression.
        The argument number is represented by N.  */
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index e09d5d6..9cc2a38 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -914,10 +914,9 @@ stap_parse_argument (const char **arg, struct type *atype,
    this information.  */
 
 static void
-stap_parse_probe_arguments (struct stap_probe *probe)
+stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 {
   const char *cur;
-  struct gdbarch *gdbarch = get_objfile_arch (probe->p.objfile);
 
   gdb_assert (!probe->args_parsed);
   cur = probe->args_u.text;
@@ -1002,16 +1001,18 @@ stap_parse_probe_arguments (struct stap_probe *probe)
    argument string.  */
 
 static unsigned
-stap_get_probe_argument_count (struct probe *probe_generic)
+stap_get_probe_argument_count (struct probe *probe_generic,
+			       struct frame_info *frame)
 {
   struct stap_probe *probe = (struct stap_probe *) probe_generic;
+  struct gdbarch *gdbarch = get_frame_arch (frame);
 
   gdb_assert (probe_generic->pops == &stap_probe_ops);
 
   if (!probe->args_parsed)
     {
-      if (probe_generic->pops->can_evaluate_probe_arguments (probe_generic))
-	stap_parse_probe_arguments (probe);
+      if (can_evaluate_probe_arguments (probe_generic))
+	stap_parse_probe_arguments (probe, gdbarch);
       else
 	{
 	  static int have_warned_stap_incomplete = 0;
@@ -1072,10 +1073,10 @@ stap_is_operator (const char *op)
 }
 
 static struct stap_probe_arg *
-stap_get_arg (struct stap_probe *probe, unsigned n)
+stap_get_arg (struct stap_probe *probe, unsigned n, struct gdbarch *gdbarch)
 {
   if (!probe->args_parsed)
-    stap_parse_probe_arguments (probe);
+    stap_parse_probe_arguments (probe, gdbarch);
 
   return VEC_index (stap_probe_arg_s, probe->args_u.vec, n);
 }
@@ -1083,10 +1084,11 @@ stap_get_arg (struct stap_probe *probe, unsigned n)
 /* Implement the `can_evaluate_probe_arguments' method of probe_ops.  */
 
 static int
-stap_can_evaluate_probe_arguments (struct probe *probe_generic)
+stap_can_evaluate_probe_arguments (struct probe *probe_generic,
+				   struct frame_info *frame)
 {
   struct stap_probe *stap_probe = (struct stap_probe *) probe_generic;
-  struct gdbarch *gdbarch = get_objfile_arch (stap_probe->p.objfile);
+  struct gdbarch *gdbarch = get_frame_arch (frame);
 
   /* For SystemTap probes, we have to guarantee that the method
      stap_is_single_operand is defined on gdbarch.  If it is not, then it
@@ -1098,15 +1100,17 @@ stap_can_evaluate_probe_arguments (struct probe *probe_generic)
    corresponding to it.  Assertion is thrown if N does not exist.  */
 
 static struct value *
-stap_evaluate_probe_argument (struct probe *probe_generic, unsigned n)
+stap_evaluate_probe_argument (struct probe *probe_generic, unsigned n,
+			      struct frame_info *frame)
 {
   struct stap_probe *stap_probe = (struct stap_probe *) probe_generic;
+  struct gdbarch *gdbarch = get_frame_arch (frame);
   struct stap_probe_arg *arg;
   int pos = 0;
 
   gdb_assert (probe_generic->pops == &stap_probe_ops);
 
-  arg = stap_get_arg (stap_probe, n);
+  arg = stap_get_arg (stap_probe, n, gdbarch);
   return evaluate_subexp_standard (arg->atype, arg->aexpr, &pos, EVAL_NORMAL);
 }
 
@@ -1123,7 +1127,7 @@ stap_compile_to_ax (struct probe *probe_generic, struct agent_expr *expr,
 
   gdb_assert (probe_generic->pops == &stap_probe_ops);
 
-  arg = stap_get_arg (stap_probe, n);
+  arg = stap_get_arg (stap_probe, n, expr->gdbarch);
 
   pc = arg->aexpr->elts;
   gen_expr (arg->aexpr, &pc, expr, value);
@@ -1165,6 +1169,7 @@ compute_probe_arg (struct gdbarch *arch, struct internalvar *ivar,
 {
   struct frame_info *frame = get_selected_frame (_("No frame selected"));
   CORE_ADDR pc = get_frame_pc (frame);
+  struct gdbarch *gdbarch = get_frame_arch (frame);
   int sel = (int) (uintptr_t) data;
   struct probe *pc_probe;
   const struct sym_probe_fns *pc_probe_fns;

Re: [PATCH v2] Sanitize access to gdbarch on the probe API (and fix ARM bug)

[Pedro Alves]

On 12/06/2013 08:31 PM, Sergio Durigan Junior wrote:
> 2013-12-06  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* probe.c (get_probe_argument_count, can_evaluate_probe_arguments)
> 	(evaluate_probe_argument): Pass selected frame to the
> 	corresponding probe_ops method being called.
> 	* probe.h (struct probe_ops) <get_probe_argument_count,
> 	can_evaluate_probe_arguments, evaluate_probe_argument>: Adjust
> 	declarations to accept frame as argument.
> 	* stap-probe.c (stap_parse_probe_arguments): Adjust declaration to
> 	accept gdbarch as argument.  Remove call to get_objfile_arch.
> 	(stap_get_probe_argument_count): Adjust declaration to accept
> 	frame as argument.  Obtain gdbarch from frame.  Call
> 	can_evaluate_probe_arguments directly instead of the corresponding
> 	probe_ops method.  Provide gdbarch to stap_parse_probe_arguments.
> 	(stap_get_arg): Adjust declaration to accept gdbarch as argument.
> 	Pass it to stap_parse_probe_arguments.
> 	(stap_can_evaluate_probe_arguments): Adjust declaration to accept
> 	frame as argument.  Obtain gdbarch from it.
> 	(stap_evaluate_probe_argument): Likewise.  Pass gdbarch to
> 	stap_get_arg.
> 	(stap_compile_to_ax): Use gdbarch from agent_expr.
> 	(compute_probe_arg): Get gdbarch from frame.
> 
> diff --git a/gdb/probe.c b/gdb/probe.c
> index c1e0111..b611282 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -616,7 +616,9 @@ info_probes_command (char *arg, int from_tty)
>  unsigned
>  get_probe_argument_count (struct probe *probe)
>  {
> -  return probe->pops->get_probe_argument_count (probe);
> +  struct frame_info *frame = get_selected_frame (_("No frame selected"));

The "No frame selected" string is actually misleading - the cause of
not being able to get the selected frame may be e.g., that the
selected thread is running.  "No frame selected" sounds to me like
it may be an old string from the old days from before GDB was
made to always have a frame (if there's stack).  I see that you
copied that from compute_probe_arg.  I'd even support going through
all get_selected_frame calls with non-NULL message, and see about
passing NULL instead (and then eliminate the parameter).

 /* Install a master breakpoint on the unwinder's debug hook.  */

 static void
 create_exception_master_breakpoint (void)
 {
   struct objfile *objfile;
   const char *const func_name = "_Unwind_DebugHook";

   ALL_OBJFILES (objfile)
     {
...
       bp_objfile_data = get_breakpoint_objfile_data (objfile);
...
	      if (!can_evaluate_probe_arguments (p))

Here, we see that the selected thread/frame is unrelated to the
probe/location can_evaluate_probe_arguments is being called
for.

[Passing the frame down from the callers (i.e., add a frame parameter
to evaluate_probe_arguments too, instead of only at the probe ops
level), would make these hidden bad couplings visible.  Can you
make that change?]

So it seems to me like the can_evaluate_probe_arguments
probe hook should work with the objfile's arch, and only
evaluation itself would use the more detailed target/frame's
gdbarch.  The get_probe_argument_count method could perhaps
follow the same rationale and use the objfile's arch.  Would
that work?

-- 
Pedro Alves

Re: [PATCH v2] Sanitize access to gdbarch on the probe API (and fix ARM bug)

[Sergio Durigan Junior]

On Monday, December 09 2013, Pedro Alves wrote:

> The "No frame selected" string is actually misleading - the cause of
> not being able to get the selected frame may be e.g., that the
> selected thread is running.  "No frame selected" sounds to me like
> it may be an old string from the old days from before GDB was
> made to always have a frame (if there's stack).  I see that you
> copied that from compute_probe_arg.  I'd even support going through
> all get_selected_frame calls with non-NULL message, and see about
> passing NULL instead (and then eliminate the parameter).

Nice, I can do that :-).

>  /* Install a master breakpoint on the unwinder's debug hook.  */
>
>  static void
>  create_exception_master_breakpoint (void)
>  {
>    struct objfile *objfile;
>    const char *const func_name = "_Unwind_DebugHook";
>
>    ALL_OBJFILES (objfile)
>      {
> ...
>        bp_objfile_data = get_breakpoint_objfile_data (objfile);
> ...
> 	      if (!can_evaluate_probe_arguments (p))
>
> Here, we see that the selected thread/frame is unrelated to the
> probe/location can_evaluate_probe_arguments is being called
> for.
>
> [Passing the frame down from the callers (i.e., add a frame parameter
> to evaluate_probe_arguments too, instead of only at the probe ops
> level), would make these hidden bad couplings visible.  Can you
> make that change?]

Yes, sure.  And thanks for explaining all this, of course.  Very
educational.

> So it seems to me like the can_evaluate_probe_arguments
> probe hook should work with the objfile's arch, and only
> evaluation itself would use the more detailed target/frame's
> gdbarch.  The get_probe_argument_count method could perhaps
> follow the same rationale and use the objfile's arch.  Would
> that work?

To be more precise, the *parsing* and the evaluation both need access to
the target/frame's gdbarch.  So, in the end, I wasn't able to make
get_probe_argument_count frame-independent; it still needs a "good"
gdbarch because, in order to count the argument, it needs to parse them
first, and parsing them first means that it will need to access
registers numbers and other "sensitive" information.

Therefore, I came up with the following patch.  It basically adds a new
"frame" argument for get_probe_argument_count and
evaluate_probe_arguments, and update all the callers.

Does that work for you?

-- 
Sergio

2013-12-10  Sergio Durigan Junior  <sergiodj@redhat.com>

	* break-catch-throw.c (fetch_probe_arguments): Pass NULL to
	get_selected_frame.  Pass selected frame to
	get_probe_argument_count and evaluate_probe_argument.
	* probe.c (get_probe_argument_count): Adjust declaration to accept
	frame.  Pass frame to probe_ops's get_probe_argument_count.
	(evaluate_probe_argument): Likewise, for evaluate_probe_argument.
	(probe_safe_evaluate_at_pc): Pass frame to
	get_probe_argument_count and evaluate_probe_argument.
	* probe.h (struct probe_ops) <get_probe_argument_count,
	evaluate_probe_argument>: Adjust declarations to accept frame.
	(get_probe_argument_count, evaluate_probe_argument): Likewise.
	* solib-svr4.c (solib_event_probe_action): Get selected frame.
	Pass it to get_probe_argument_count.
	(svr4_handle_solib_event): Get selected frame.  Pass it to
	get_probe_argument_count and evaluate_probe_argument.
	* stap-probe.c (stap_parse_probe_arguments): Adjust declaration to
	accept gdbarch.  Do not obtain it from the probe's objfile.
	(stap_get_probe_argument_count): Adjust declaration to accept
	frame.  Obtain gdbarch from the frame.  Call generic
	can_evaluate_probe_arguments.  Pass gdbarch to
	stap_parse_probe_arguments.
	(stap_get_arg): Adjust declaration to accept gdbarch.  Pass it to
	stap_parse_probe_arguments.
	(stap_evaluate_probe_argument): Adjust declaration to accept
	frame.  Obtain gdbarch from the frame.  Pass gdbarch to
	stap_get_arg.
	(stap_compile_to_ax): Pass agent_expr's gdbarch to stap_get_arg.
	(compute_probe_arg): Pass NULL to get_selected_frame.  Obtain
	gdbarch from frame.  Pass frame to get_probe_argument_count and
	evaluate_probe_argument.

diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 76087d3..cb036da 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -104,7 +104,7 @@ struct exception_catchpoint
 static void
 fetch_probe_arguments (struct value **arg0, struct value **arg1)
 {
-  struct frame_info *frame = get_selected_frame (_("No frame selected"));
+  struct frame_info *frame = get_selected_frame (NULL);
   CORE_ADDR pc = get_frame_pc (frame);
   struct probe *pc_probe;
   const struct sym_probe_fns *pc_probe_fns;
@@ -118,13 +118,13 @@ fetch_probe_arguments (struct value **arg0, struct value **arg1)
 	  && strcmp (pc_probe->name, "rethrow") != 0))
     error (_("not stopped at a C++ exception catchpoint"));
 
-  n_args = get_probe_argument_count (pc_probe);
+  n_args = get_probe_argument_count (pc_probe, frame);
   if (n_args < 2)
     error (_("C++ exception catchpoint has too few arguments"));
 
   if (arg0 != NULL)
-    *arg0 = evaluate_probe_argument (pc_probe, 0);
-  *arg1 = evaluate_probe_argument (pc_probe, 1);
+    *arg0 = evaluate_probe_argument (pc_probe, 0, frame);
+  *arg1 = evaluate_probe_argument (pc_probe, 1, frame);
 
   if ((arg0 != NULL && *arg0 == NULL) || *arg1 == NULL)
     error (_("error computing probe argument at c++ exception catchpoint"));
diff --git a/gdb/probe.c b/gdb/probe.c
index c1e0111..8dad364 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -614,9 +614,9 @@ info_probes_command (char *arg, int from_tty)
 /* See comments in probe.h.  */
 
 unsigned
-get_probe_argument_count (struct probe *probe)
+get_probe_argument_count (struct probe *probe, struct frame_info *frame)
 {
-  return probe->pops->get_probe_argument_count (probe);
+  return probe->pops->get_probe_argument_count (probe, frame);
 }
 
 /* See comments in probe.h.  */
@@ -630,9 +630,10 @@ can_evaluate_probe_arguments (struct probe *probe)
 /* See comments in probe.h.  */
 
 struct value *
-evaluate_probe_argument (struct probe *probe, unsigned n)
+evaluate_probe_argument (struct probe *probe, unsigned n,
+			 struct frame_info *frame)
 {
-  return probe->pops->evaluate_probe_argument (probe, n);
+  return probe->pops->evaluate_probe_argument (probe, n, frame);
 }
 
 /* See comments in probe.h.  */
@@ -647,11 +648,11 @@ probe_safe_evaluate_at_pc (struct frame_info *frame, unsigned n)
   if (!probe)
     return NULL;
 
-  n_args = get_probe_argument_count (probe);
+  n_args = get_probe_argument_count (probe, frame);
   if (n >= n_args)
     return NULL;
 
-  return evaluate_probe_argument (probe, n);
+  return evaluate_probe_argument (probe, n, frame);
 }
 
 /* See comment in probe.h.  */
diff --git a/gdb/probe.h b/gdb/probe.h
index dd5387b..daf2573 100644
--- a/gdb/probe.h
+++ b/gdb/probe.h
@@ -71,7 +71,8 @@ struct probe_ops
 
     /* Return the number of arguments of PROBE.  */
 
-    unsigned (*get_probe_argument_count) (struct probe *probe);
+    unsigned (*get_probe_argument_count) (struct probe *probe,
+					  struct frame_info *frame);
 
     /* Return 1 if the probe interface can evaluate the arguments of probe
        PROBE, zero otherwise.  See the comments on
@@ -83,7 +84,8 @@ struct probe_ops
        corresponding to it.  The argument number is represented N.  */
 
     struct value *(*evaluate_probe_argument) (struct probe *probe,
-					      unsigned n);
+					      unsigned n,
+					      struct frame_info *frame);
 
     /* Compile the Nth argument of the PROBE to an agent expression.
        The argument number is represented by N.  */
@@ -222,7 +224,8 @@ extern struct cmd_list_element **info_probes_cmdlist_get (void);
 
 /* Return the argument count of the specified probe.  */
 
-extern unsigned get_probe_argument_count (struct probe *probe);
+extern unsigned get_probe_argument_count (struct probe *probe,
+					  struct frame_info *frame);
 
 /* Return 1 if the probe interface associated with PROBE can evaluate
    arguments, zero otherwise.  See the comments on the definition of
@@ -234,7 +237,8 @@ extern int can_evaluate_probe_arguments (struct probe *probe);
    inclusive and get_probe_argument_count exclusive.  */
 
 extern struct value *evaluate_probe_argument (struct probe *probe,
-					      unsigned n);
+					      unsigned n,
+					      struct frame_info *frame);
 
 /* A convenience function that finds a probe at the PC in FRAME and
    evaluates argument N, with 0 <= N < number_of_args.  If there is no
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 9538af6..8ffef57 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1651,6 +1651,7 @@ solib_event_probe_action (struct probe_and_action *pa)
 {
   enum probe_action action;
   unsigned probe_argc;
+  struct frame_info *frame = get_selected_frame (NULL);
 
   action = pa->action;
   if (action == DO_NOTHING || action == PROBES_INTERFACE_FAILED)
@@ -1663,7 +1664,7 @@ solib_event_probe_action (struct probe_and_action *pa)
        arg0: Lmid_t lmid (mandatory)
        arg1: struct r_debug *debug_base (mandatory)
        arg2: struct link_map *new (optional, for incremental updates)  */
-  probe_argc = get_probe_argument_count (pa->probe);
+  probe_argc = get_probe_argument_count (pa->probe, frame);
   if (probe_argc == 2)
     action = FULL_RELOAD;
   else if (probe_argc < 2)
@@ -1772,6 +1773,7 @@ svr4_handle_solib_event (void)
   struct value *val;
   CORE_ADDR pc, debug_base, lm = 0;
   int is_initial_ns;
+  struct frame_info *frame = get_selected_frame (NULL);
 
   /* Do nothing if not using the probes interface.  */
   if (info->probes_table == NULL)
@@ -1816,7 +1818,7 @@ svr4_handle_solib_event (void)
   usm_chain = make_cleanup (resume_section_map_updates_cleanup,
 			    current_program_space);
 
-  val = evaluate_probe_argument (pa->probe, 1);
+  val = evaluate_probe_argument (pa->probe, 1, frame);
   if (val == NULL)
     {
       do_cleanups (old_chain);
@@ -1847,7 +1849,7 @@ svr4_handle_solib_event (void)
 
   if (action == UPDATE_OR_RELOAD)
     {
-      val = evaluate_probe_argument (pa->probe, 2);
+      val = evaluate_probe_argument (pa->probe, 2, frame);
       if (val != NULL)
 	lm = value_as_address (val);
 
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index e09d5d6..4a8d191 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -914,10 +914,9 @@ stap_parse_argument (const char **arg, struct type *atype,
    this information.  */
 
 static void
-stap_parse_probe_arguments (struct stap_probe *probe)
+stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 {
   const char *cur;
-  struct gdbarch *gdbarch = get_objfile_arch (probe->p.objfile);
 
   gdb_assert (!probe->args_parsed);
   cur = probe->args_u.text;
@@ -1002,16 +1001,18 @@ stap_parse_probe_arguments (struct stap_probe *probe)
    argument string.  */
 
 static unsigned
-stap_get_probe_argument_count (struct probe *probe_generic)
+stap_get_probe_argument_count (struct probe *probe_generic,
+			       struct frame_info *frame)
 {
   struct stap_probe *probe = (struct stap_probe *) probe_generic;
+  struct gdbarch *gdbarch = get_frame_arch (frame);
 
   gdb_assert (probe_generic->pops == &stap_probe_ops);
 
   if (!probe->args_parsed)
     {
-      if (probe_generic->pops->can_evaluate_probe_arguments (probe_generic))
-	stap_parse_probe_arguments (probe);
+      if (can_evaluate_probe_arguments (probe_generic))
+	stap_parse_probe_arguments (probe, gdbarch);
       else
 	{
 	  static int have_warned_stap_incomplete = 0;
@@ -1072,10 +1073,10 @@ stap_is_operator (const char *op)
 }
 
 static struct stap_probe_arg *
-stap_get_arg (struct stap_probe *probe, unsigned n)
+stap_get_arg (struct stap_probe *probe, unsigned n, struct gdbarch *gdbarch)
 {
   if (!probe->args_parsed)
-    stap_parse_probe_arguments (probe);
+    stap_parse_probe_arguments (probe, gdbarch);
 
   return VEC_index (stap_probe_arg_s, probe->args_u.vec, n);
 }
@@ -1098,15 +1099,17 @@ stap_can_evaluate_probe_arguments (struct probe *probe_generic)
    corresponding to it.  Assertion is thrown if N does not exist.  */
 
 static struct value *
-stap_evaluate_probe_argument (struct probe *probe_generic, unsigned n)
+stap_evaluate_probe_argument (struct probe *probe_generic, unsigned n,
+			      struct frame_info *frame)
 {
   struct stap_probe *stap_probe = (struct stap_probe *) probe_generic;
+  struct gdbarch *gdbarch = get_frame_arch (frame);
   struct stap_probe_arg *arg;
   int pos = 0;
 
   gdb_assert (probe_generic->pops == &stap_probe_ops);
 
-  arg = stap_get_arg (stap_probe, n);
+  arg = stap_get_arg (stap_probe, n, gdbarch);
   return evaluate_subexp_standard (arg->atype, arg->aexpr, &pos, EVAL_NORMAL);
 }
 
@@ -1123,7 +1126,7 @@ stap_compile_to_ax (struct probe *probe_generic, struct agent_expr *expr,
 
   gdb_assert (probe_generic->pops == &stap_probe_ops);
 
-  arg = stap_get_arg (stap_probe, n);
+  arg = stap_get_arg (stap_probe, n, expr->gdbarch);
 
   pc = arg->aexpr->elts;
   gen_expr (arg->aexpr, &pc, expr, value);
@@ -1163,7 +1166,7 @@ static struct value *
 compute_probe_arg (struct gdbarch *arch, struct internalvar *ivar,
 		   void *data)
 {
-  struct frame_info *frame = get_selected_frame (_("No frame selected"));
+  struct frame_info *frame = get_selected_frame (NULL);
   CORE_ADDR pc = get_frame_pc (frame);
   int sel = (int) (uintptr_t) data;
   struct probe *pc_probe;
@@ -1177,7 +1180,7 @@ compute_probe_arg (struct gdbarch *arch, struct internalvar *ivar,
   if (pc_probe == NULL)
     error (_("No SystemTap probe at PC %s"), core_addr_to_string (pc));
 
-  n_args = get_probe_argument_count (pc_probe);
+  n_args = get_probe_argument_count (pc_probe, frame);
   if (sel == -1)
     return value_from_longest (builtin_type (arch)->builtin_int, n_args);
 
@@ -1185,7 +1188,7 @@ compute_probe_arg (struct gdbarch *arch, struct internalvar *ivar,
     error (_("Invalid probe argument %d -- probe has %u arguments available"),
 	   sel, n_args);
 
-  return evaluate_probe_argument (pc_probe, sel);
+  return evaluate_probe_argument (pc_probe, sel, frame);
 }
 
 /* This is called to compile one of the $_probe_arg* convenience
@@ -1200,6 +1203,7 @@ compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
   struct probe *pc_probe;
   const struct sym_probe_fns *pc_probe_fns;
   int n_args;
+  struct frame_info *frame = get_selected_frame (NULL);
 
   /* SEL == -1 means "_probe_argc".  */
   gdb_assert (sel >= -1);
@@ -1208,7 +1212,7 @@ compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
   if (pc_probe == NULL)
     error (_("No SystemTap probe at PC %s"), core_addr_to_string (pc));
 
-  n_args = get_probe_argument_count (pc_probe);
+  n_args = get_probe_argument_count (pc_probe, frame);
 
   if (sel == -1)
     {

Re: [PATCH v2] Sanitize access to gdbarch on the probe API (and fix ARM bug)

[Pedro Alves]

On 12/10/2013 04:04 AM, Sergio Durigan Junior wrote:
> --- a/gdb/break-catch-throw.c
> +++ b/gdb/break-catch-throw.c
> @@ -104,7 +104,7 @@ struct exception_catchpoint
>  static void
>  fetch_probe_arguments (struct value **arg0, struct value **arg1)
>  {
> -  struct frame_info *frame = get_selected_frame (_("No frame selected"));
> +  struct frame_info *frame = get_selected_frame (NULL);
>    CORE_ADDR pc = get_frame_pc (frame);

The bits that do this change are OK, though I'd have preferred they
were split to a separate, preparatory patch, and checked in as
a separate commit, with its own rationale.

On 12/10/2013 04:04 AM, Sergio Durigan Junior wrote:
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -1651,6 +1651,7 @@ solib_event_probe_action (struct probe_and_action *pa)
>  {
>    enum probe_action action;
>    unsigned probe_argc;
> +  struct frame_info *frame = get_selected_frame (NULL);

Missed a little detail here before.  Strictly speaking, event handling
functions always work with the current, innermost frame, as that
always corresponds to where the program actually stopped.  In
that sense, writing that out explicitly is more self-documenting:

  struct frame_info *frame = get_current_frame ();

>  
>    action = pa->action;
>    if (action == DO_NOTHING || action == PROBES_INTERFACE_FAILED)
> @@ -1663,7 +1664,7 @@ solib_event_probe_action (struct probe_and_action *pa)
>         arg0: Lmid_t lmid (mandatory)
>         arg1: struct r_debug *debug_base (mandatory)
>         arg2: struct link_map *new (optional, for incremental updates)  */
> -  probe_argc = get_probe_argument_count (pa->probe);
> +  probe_argc = get_probe_argument_count (pa->probe, frame);
>    if (probe_argc == 2)
>      action = FULL_RELOAD;
>    else if (probe_argc < 2)
> @@ -1772,6 +1773,7 @@ svr4_handle_solib_event (void)
>    struct value *val;
>    CORE_ADDR pc, debug_base, lm = 0;
>    int is_initial_ns;
> +  struct frame_info *frame = get_selected_frame (NULL);

Here too.

Otherwise looks good to me.

Thanks,
-- 
Pedro Alves

Re: [PATCH v2] Sanitize access to gdbarch on the probe API (and fix ARM bug)

[Sergio Durigan Junior]

On Tuesday, December 10 2013, Pedro Alves wrote:

> On 12/10/2013 04:04 AM, Sergio Durigan Junior wrote:
>> --- a/gdb/break-catch-throw.c
>> +++ b/gdb/break-catch-throw.c
>> @@ -104,7 +104,7 @@ struct exception_catchpoint
>>  static void
>>  fetch_probe_arguments (struct value **arg0, struct value **arg1)
>>  {
>> -  struct frame_info *frame = get_selected_frame (_("No frame selected"));
>> +  struct frame_info *frame = get_selected_frame (NULL);
>>    CORE_ADDR pc = get_frame_pc (frame);
>
> The bits that do this change are OK, though I'd have preferred they
> were split to a separate, preparatory patch, and checked in as
> a separate commit, with its own rationale.

Right, I've removed those bits from the patch.

I will probably open another thread or ping you on IRC to discuss the
cleanup of get_selected_frame.

> On 12/10/2013 04:04 AM, Sergio Durigan Junior wrote:
>> --- a/gdb/solib-svr4.c
>> +++ b/gdb/solib-svr4.c
>> @@ -1651,6 +1651,7 @@ solib_event_probe_action (struct probe_and_action *pa)
>>  {
>>    enum probe_action action;
>>    unsigned probe_argc;
>> +  struct frame_info *frame = get_selected_frame (NULL);
>
> Missed a little detail here before.  Strictly speaking, event handling
> functions always work with the current, innermost frame, as that
> always corresponds to where the program actually stopped.  In
> that sense, writing that out explicitly is more self-documenting:
>
>   struct frame_info *frame = get_current_frame ();

Fixed.

>>  
>>    action = pa->action;
>>    if (action == DO_NOTHING || action == PROBES_INTERFACE_FAILED)
>> @@ -1663,7 +1664,7 @@ solib_event_probe_action (struct probe_and_action *pa)
>>         arg0: Lmid_t lmid (mandatory)
>>         arg1: struct r_debug *debug_base (mandatory)
>>         arg2: struct link_map *new (optional, for incremental updates)  */
>> -  probe_argc = get_probe_argument_count (pa->probe);
>> +  probe_argc = get_probe_argument_count (pa->probe, frame);
>>    if (probe_argc == 2)
>>      action = FULL_RELOAD;
>>    else if (probe_argc < 2)
>> @@ -1772,6 +1773,7 @@ svr4_handle_solib_event (void)
>>    struct value *val;
>>    CORE_ADDR pc, debug_base, lm = 0;
>>    int is_initial_ns;
>> +  struct frame_info *frame = get_selected_frame (NULL);
>
> Here too.

Fixed.

> Otherwise looks good to me.

Thanks.  Patch checked-in:

         https://sourceware.org/ml/gdb-cvs/2013-12/msg00065.html

-- 
Sergio