Re: [PATCH] Implement way of checking if probe interface can evaluate arguments

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Wednesday, July 17 2013, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> Yes, I wanted to check only one probe.
>
> I think the crux is this:
>
> Sergio> +  /* For SystemTap probes, we have to guarantee that the method
> Sergio> +     stap_is_single_operand is defined on gdbarch.  If it is not, then it
> Sergio> +     means that argument evaluation is not implemented on this target.  */
> Sergio> +  return gdbarch_stap_is_single_operand_p (gdbarch);
>
> The question I have is whether this is sufficient.
>
> I was picturing checking whether each particular probe can be parsed.
> However, perhaps the above is enough -- you tell me.

Thanks for the review.

Well, there are two things here.

The first one is whether the argument evaluation is implemented in the
target (which is what I'm checking in the function above).  If it is
not, then we issue the message and don't try to evaluate anything
probe-related again.  This is a bug, but only because the support is not
implemented in the target yet.

The second thing is whether each particular argument of each probe can
be parsed.  In this scenario, the target support for evaluating
arguments is implemented, but the parser still can't handle some
particular argument.  This, IMO, is a bug more severe/important than the
one described above, because the argument parser is broken.  And if it
happens, GDB will display a message saying that it was unable to
parse/evaluate the argument, and it will print the full argument in the
message.

Having said that, the answer to your question is "the above is enough
*when we are checking if the target provides support for the parsing of
arguments*".  But since the code already covers the case when the
parsing fails because the parser is broken (second case), I think we
don't need to worry about verifying each probe separately.

> Sergio> +"you will not be able to inspect probes's arguments.\n"
>
> I think "the arguments of the probes" sounds less weird.

Fixed.

> Sergio> +"Please, report a bug against GDB requesting for the implementation to be\n"
>
> I would drop the comma after "Please" and write:
>
>   Please report a bug against GDB requesting a port to this target.

Fixed.

> Also, does this warning show up if gdb tries to use the probes
> automatically?  I think for that case we will need a separate warning of
> some kind, since the user probably doesn't even know probes are
> involved.

This warning will only be displayed when GDB tries to evaluate the
arguments of the probes.  Here's what the user will see for example when
she starts GDB and it tries to use the probes-based dynamic linker
interface:

  (gdb) start
  Temporary breakpoint 1 at 0x4005b9: file ../../../gdb-src-stap-warning/gdb/testsuite/gdb.base/stap-probe.c, line 97.
  Starting program: /home/sergio/work/src/git/stap-warning/build-64/gdb/testsuite/gdb.base/stap-probe-nosem-noopt 
  warning: Skipping deprecated .gdb_index section in /usr/lib/debug/lib64/ld-2.15.so.debug.
  Do "set use-deprecated-index-sections on" before the file is read
  to use the section anyway.
  warning: The SystemTap SDT probe support is not fully implemented on this target;
  you will not be able to inspect the arguments of the probes.
  Please report a bug against GDB requesting a port to this target.
  warning: Probes-based dynamic linker interface failed.
  Reverting to original interface.

The first warning may be confusing because of what you said, but the
second warning (displayed by Gary's patch) clarifies that the dynamic
linker interface is probes-based.

WDYT?

-- 
Sergio

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 4d09b30..1e89407 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3194,8 +3194,23 @@ create_longjmp_master_breakpoint (void)
 
       if (!bp_objfile_data->longjmp_searched)
 	{
-	  bp_objfile_data->longjmp_probes
-	    = find_probes_in_objfile (objfile, "libc", "longjmp");
+	  VEC (probe_p) *ret;
+
+	  ret = find_probes_in_objfile (objfile, "libc", "longjmp");
+	  if (ret != NULL)
+	    {
+	      /* We are only interested in checking one element.  */
+	      struct probe *p = VEC_index (probe_p, ret, 0);
+
+	      if (!can_evaluate_probe_arguments (p))
+		{
+		  /* We cannot use the probe interface here, because it does
+		     not know how to evaluate arguments.  */
+		  VEC_free (probe_p, ret);
+		  ret = NULL;
+		}
+	    }
+	  bp_objfile_data->longjmp_probes = ret;
 	  bp_objfile_data->longjmp_searched = 1;
 	}
 
@@ -3336,8 +3351,24 @@ create_exception_master_breakpoint (void)
       /* We prefer the SystemTap probe point if it exists.  */
       if (!bp_objfile_data->exception_searched)
 	{
-	  bp_objfile_data->exception_probes
-	    = find_probes_in_objfile (objfile, "libgcc", "unwind");
+	  VEC (probe_p) *ret;
+
+	  ret = find_probes_in_objfile (objfile, "libgcc", "unwind");
+
+	  if (ret != NULL)
+	    {
+	      /* We are only interested in checking one element.  */
+	      struct probe *p = VEC_index (probe_p, ret, 0);
+
+	      if (!can_evaluate_probe_arguments (p))
+		{
+		  /* We cannot use the probe interface here, because it does
+		     not know how to evaluate arguments.  */
+		  VEC_free (probe_p, ret);
+		  ret = NULL;
+		}
+	    }
+	  bp_objfile_data->exception_probes = ret;
 	  bp_objfile_data->exception_searched = 1;
 	}
 
diff --git a/gdb/elfread.c b/gdb/elfread.c
index cfdfe45..1aa10d1 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1643,6 +1643,15 @@ elf_get_probe_argument_count (struct probe *probe)
   return probe->pops->get_probe_argument_count (probe);
 }
 
+/* Implementation of `sym_can_evaluate_probe_arguments', as documented in
+   symfile.h.  */
+
+static int
+elf_can_evaluate_probe_arguments (struct probe *probe)
+{
+  return probe->pops->can_evaluate_probe_arguments (probe);
+}
+
 /* Implementation of `sym_evaluate_probe_argument', as documented in
    symfile.h.  */
 
@@ -1700,11 +1709,12 @@ probe_key_free (struct objfile *objfile, void *d)
 
 static const struct sym_probe_fns elf_probe_fns =
 {
-  elf_get_probes,		/* sym_get_probes */
-  elf_get_probe_argument_count,	/* sym_get_probe_argument_count */
-  elf_evaluate_probe_argument,	/* sym_evaluate_probe_argument */
-  elf_compile_to_ax,		/* sym_compile_to_ax */
-  elf_symfile_relocate_probe,	/* sym_relocate_probe */
+  elf_get_probes,		    /* sym_get_probes */
+  elf_get_probe_argument_count,	    /* sym_get_probe_argument_count */
+  elf_can_evaluate_probe_arguments, /* sym_can_evaluate_probe_arguments */
+  elf_evaluate_probe_argument,	    /* sym_evaluate_probe_argument */
+  elf_compile_to_ax,		    /* sym_compile_to_ax */
+  elf_symfile_relocate_probe,	    /* sym_relocate_probe */
 };
 
 /* Register that we are able to handle ELF object file formats.  */
diff --git a/gdb/probe.c b/gdb/probe.c
index e650892..c313c38 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -628,6 +628,23 @@ get_probe_argument_count (struct probe *probe)
 
 /* See comments in probe.h.  */
 
+int
+can_evaluate_probe_arguments (struct probe *probe)
+{
+  const struct sym_probe_fns *probe_fns;
+
+  gdb_assert (probe->objfile != NULL);
+  gdb_assert (probe->objfile->sf != NULL);
+
+  probe_fns = probe->objfile->sf->sym_probe_fns;
+
+  gdb_assert (probe_fns != NULL);
+
+  return probe_fns->can_evaluate_probe_arguments (probe);
+}
+
+/* See comments in probe.h.  */
+
 struct value *
 evaluate_probe_argument (struct probe *probe, unsigned n)
 {
diff --git a/gdb/probe.h b/gdb/probe.h
index de07f50..dd5387b 100644
--- a/gdb/probe.h
+++ b/gdb/probe.h
@@ -73,6 +73,12 @@ struct probe_ops
 
     unsigned (*get_probe_argument_count) (struct probe *probe);
 
+    /* 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);
+
     /* Evaluate the Nth argument from the PROBE, returning a value
        corresponding to it.  The argument number is represented N.  */
 
@@ -218,6 +224,12 @@ extern struct cmd_list_element **info_probes_cmdlist_get (void);
 
 extern unsigned get_probe_argument_count (struct probe *probe);
 
+/* Return 1 if the probe interface associated with PROBE can evaluate
+   arguments, zero otherwise.  See the comments on the definition of
+   sym_probe_fns:can_evaluate_probe_arguments for more details.  */
+
+extern int can_evaluate_probe_arguments (struct probe *probe);
+
 /* Evaluate argument N of the specified probe.  N must be between 0
    inclusive and get_probe_argument_count exclusive.  */
 
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index ccfd158..a497c6c 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1978,12 +1978,14 @@ svr4_create_solib_event_breakpoints (struct gdbarch *gdbarch,
 	{
 	  VEC (probe_p) *probes[NUM_PROBES];
 	  int all_probes_found = 1;
+	  int checked_can_use_probe_arguments = 0;
 	  int i;
 
 	  memset (probes, 0, sizeof (probes));
 	  for (i = 0; i < NUM_PROBES; i++)
 	    {
 	      const char *name = probe_info[i].name;
+	      struct probe *p;
 	      char buf[32];
 
 	      /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
@@ -2012,6 +2014,18 @@ svr4_create_solib_event_breakpoints (struct gdbarch *gdbarch,
 		  all_probes_found = 0;
 		  break;
 		}
+
+	      /* Ensure probe arguments can be evaluated.  */
+	      if (!checked_can_use_probe_arguments)
+		{
+		  p = VEC_index (probe_p, probes[i], 0);
+		  if (!can_evaluate_probe_arguments (p))
+		    {
+		      all_probes_found = 0;
+		      break;
+		    }
+		  checked_can_use_probe_arguments = 1;
+		}
 	    }
 
 	  if (all_probes_found)
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index 1079e3b..b2e7e6a 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -1009,7 +1009,27 @@ stap_get_probe_argument_count (struct probe *probe_generic)
   gdb_assert (probe_generic->pops == &stap_probe_ops);
 
   if (!probe->args_parsed)
-    stap_parse_probe_arguments (probe);
+    {
+      if (!probe_generic->pops->can_evaluate_probe_arguments (probe_generic))
+	stap_parse_probe_arguments (probe);
+      else
+	{
+	  static int have_warned_stap_incomplete = 0;
+
+	  if (!have_warned_stap_incomplete)
+	    {
+	      warning (_(
+"The SystemTap SDT probe support is not fully implemented on this target;\n"
+"you will not be able to inspect the arguments of the probes.\n"
+"Please report a bug against GDB requesting a port to this target."));
+	      have_warned_stap_incomplete = 1;
+	    }
+
+	  /* Marking the arguments as "already parsed".  */
+	  probe->args_u.vec = NULL;
+	  probe->args_parsed = 1;
+	}
+    }
 
   gdb_assert (probe->args_parsed);
   return VEC_length (stap_probe_arg_s, probe->args_u.vec);
@@ -1060,6 +1080,20 @@ stap_get_arg (struct stap_probe *probe, unsigned n)
   return VEC_index (stap_probe_arg_s, probe->args_u.vec, n);
 }
 
+/* Implement the `can_evaluate_probe_arguments' method of probe_ops.  */
+
+static int
+stap_can_evaluate_probe_arguments (struct probe *probe_generic)
+{
+  struct stap_probe *stap_probe = (struct stap_probe *) probe_generic;
+  struct gdbarch *gdbarch = stap_probe->p.objfile->gdbarch;
+
+  /* For SystemTap probes, we have to guarantee that the method
+     stap_is_single_operand is defined on gdbarch.  If it is not, then it
+     means that argument evaluation is not implemented on this target.  */
+  return gdbarch_stap_is_single_operand_p (gdbarch);
+}
+
 /* Evaluate the probe's argument N (indexed from 0), returning a value
    corresponding to it.  Assertion is thrown if N does not exist.  */
 
@@ -1520,6 +1554,7 @@ static const struct probe_ops stap_probe_ops =
   stap_get_probes,
   stap_relocate,
   stap_get_probe_argument_count,
+  stap_can_evaluate_probe_arguments,
   stap_evaluate_probe_argument,
   stap_compile_to_ax,
   stap_set_semaphore,
diff --git a/gdb/symfile.h b/gdb/symfile.h
index c0e367d..c36e6b3 100644
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -315,6 +315,14 @@ struct sym_probe_fns
      implement this method as well.  */
   unsigned (*sym_get_probe_argument_count) (struct probe *probe);
 
+  /* Return 1 if the probe interface can evaluate the arguments of probe
+     PROBE, zero otherwise.  This function can be probe-specific, informing
+     whether only the arguments of PROBE can be evaluated, of generic,
+     informing whether the probe interface is able to evaluate any kind of
+     argument.  If you provide an implementation of sym_get_probes, you must
+     implement this method as well.  */
+  int (*can_evaluate_probe_arguments) (struct probe *probe);
+
   /* Evaluate the Nth argument available to PROBE.  PROBE will have
      come from a call to this objfile's sym_get_probes method.  N will
      be between 0 and the number of arguments available to this probe.