This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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)
     {


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]