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

[Pedro Alves <palves at redhat dot com>]

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