[PATCH v2 0/3] Fix stop-on-solib event failures

[PATCH v2 0/3] Fix stop-on-solib event failures

[Alan Hayward]

On some Arm targets (namely the buildbot Arm Docker setup) placing breakpoints
on just the solib dynamic probes will cause the target process to not stop.
This is due to the probes being invalid - see link in 3/3 for more details.

Fix is to fully validate the probes before using the,.

Patches 1 and 2 are code refactors. The actual fix is in patch 3.


Alan Hayward (3):
  Refactor svr4_create_solib_event_breakpoints
  Use gdbarch for probe::get_argument_count
  Check arguments for all probes before using them

 gdb/break-catch-throw.c |   2 +-
 gdb/dtrace-probe.c      |   4 +-
 gdb/probe.c             |   7 +-
 gdb/probe.h             |   2 +-
 gdb/solib-svr4.c        | 139 ++++++++++++++++++++--------------------
 gdb/stap-probe.c        |   6 +-
 6 files changed, 80 insertions(+), 80 deletions(-)

-- 
2.20.1 (Apple Git-117)

[PATCH v2 1/3] Refactor svr4_create_solib_event_breakpoints

[Alan Hayward]

Move the bulk of svr4_create_solib_event_breakpoints into a new
function to simplify the logic. No functional changes.

gdb/ChangeLog:

2019-08-21  Alan Hayward  <alan.hayward@arm.com>

	* solib-svr4.c (svr4_find_and_create_probe_breakpoints): Move
	code to here...
	(svr4_create_solib_event_breakpoints): ...from here.
---
 gdb/solib-svr4.c | 127 ++++++++++++++++++++++-------------------------
 1 file changed, 60 insertions(+), 67 deletions(-)

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index c0c505acaa..b21eacb68f 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -2061,6 +2061,61 @@ svr4_create_probe_breakpoints (svr4_info *info, struct gdbarch *gdbarch,
   svr4_update_solib_event_breakpoints ();
 }
 
+/* Find all the glibc named probes.  Only if all of the probes are found, then
+   create them and return true.  Otherwise return false.  If WITH_PREFIX is set
+   then add "rtld" to the front of the probe names.  */
+static bool
+svr4_find_and_create_probe_breakpoints (svr4_info *info,
+					struct gdbarch *gdbarch,
+					struct obj_section *os,
+					bool with_prefix)
+{
+  std::vector<probe *> probes[NUM_PROBES];
+  bool checked_can_use_probe_arguments = false;
+
+  for (int i = 0; i < NUM_PROBES; i++)
+    {
+      const char *name = probe_info[i].name;
+      char buf[32];
+
+      /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4 shipped with an early
+	 version of the probes code in which the probes' names were prefixed
+	 with "rtld_" and the "map_failed" probe did not exist.  The locations
+	 of the probes are otherwise the same, so we check for probes with
+	 prefixed names if probes with unprefixed names are not present.  */
+      if (with_prefix)
+	{
+	  xsnprintf (buf, sizeof (buf), "rtld_%s", name);
+	  name = buf;
+	}
+
+      probes[i] = find_probes_in_objfile (os->objfile, "rtld", name);
+
+      /* The "map_failed" probe did not exist in early
+	 versions of the probes code in which the probes'
+	 names were prefixed with "rtld_".  */
+      if (with_prefix && streq (name, "rtld_map_failed"))
+	continue;
+
+      /* Ensure at least one probe for the current name was found.  */
+      if (probes[i].empty ())
+	return false;
+
+      /* Ensure probe arguments can be evaluated.  */
+      if (!checked_can_use_probe_arguments)
+	{
+	  probe *p = probes[i][0];
+	  if (!p->can_evaluate_arguments ())
+	    return false;
+	  checked_can_use_probe_arguments = true;
+	}
+    }
+
+  /* All probes found.  Now create them.  */
+  svr4_create_probe_breakpoints (info, gdbarch, probes, os->objfile);
+  return true;
+}
+
 /* Both the SunOS and the SVR4 dynamic linkers call a marker function
    before and after mapping and unmapping shared libraries.  The sole
    purpose of this method is to allow debuggers to set a breakpoint so
@@ -2077,74 +2132,12 @@ static void
 svr4_create_solib_event_breakpoints (svr4_info *info, struct gdbarch *gdbarch,
 				     CORE_ADDR address)
 {
-  struct obj_section *os;
-
-  os = find_pc_section (address);
-  if (os != NULL)
-    {
-      int with_prefix;
-
-      for (with_prefix = 0; with_prefix <= 1; with_prefix++)
-	{
-	  std::vector<probe *> probes[NUM_PROBES];
-	  int all_probes_found = 1;
-	  int checked_can_use_probe_arguments = 0;
-
-	  for (int i = 0; i < NUM_PROBES; i++)
-	    {
-	      const char *name = probe_info[i].name;
-	      probe *p;
-	      char buf[32];
-
-	      /* Fedora 17 and Red Hat Enterprise Linux 6.2-6.4
-		 shipped with an early version of the probes code in
-		 which the probes' names were prefixed with "rtld_"
-		 and the "map_failed" probe did not exist.  The
-		 locations of the probes are otherwise the same, so
-		 we check for probes with prefixed names if probes
-		 with unprefixed names are not present.  */
-	      if (with_prefix)
-		{
-		  xsnprintf (buf, sizeof (buf), "rtld_%s", name);
-		  name = buf;
-		}
-
-	      probes[i] = find_probes_in_objfile (os->objfile, "rtld", name);
-
-	      /* The "map_failed" probe did not exist in early
-		 versions of the probes code in which the probes'
-		 names were prefixed with "rtld_".  */
-	      if (strcmp (name, "rtld_map_failed") == 0)
-		continue;
-
-	      if (probes[i].empty ())
-		{
-		  all_probes_found = 0;
-		  break;
-		}
-
-	      /* Ensure probe arguments can be evaluated.  */
-	      if (!checked_can_use_probe_arguments)
-		{
-		  p = probes[i][0];
-		  if (!p->can_evaluate_arguments ())
-		    {
-		      all_probes_found = 0;
-		      break;
-		    }
-		  checked_can_use_probe_arguments = 1;
-		}
-	    }
-
-	  if (all_probes_found)
-	    svr4_create_probe_breakpoints (info, gdbarch, probes, os->objfile);
-
-	  if (all_probes_found)
-	    return;
-	}
-    }
+  struct obj_section *os = find_pc_section (address);
 
-  create_solib_event_breakpoint (gdbarch, address);
+  if (os == nullptr
+      || (!svr4_find_and_create_probe_breakpoints (info, gdbarch, os, false)
+	  && !svr4_find_and_create_probe_breakpoints (info, gdbarch, os, true)))
+    create_solib_event_breakpoint (gdbarch, address);
 }
 
 /* Helper function for gdb_bfd_lookup_symbol.  */
-- 
2.20.1 (Apple Git-117)

[PATCH v2 3/3] Check arguments for all probes before using them

[Alan Hayward]

There is a long standing bug in the Arm toolchain where invalid
stap probes get created due to the probes referring to symbols which
have been resolved away.

More details are here:
https://bugzilla.redhat.com/show_bug.cgi?id=1196181

When these invalid probes are present, GDB will create the breakpoints
and then fail to stop. The errors are only spotted the first time
GDB stops, which is too late.

The solution is to ensure the arguments for all the probes are
resolved before using them.

This fixes >100 timeouts when running break-interp.exp when using
bad probes.

gdb/ChangeLog:

2019-08-21  Alan Hayward  <alan.hayward@arm.com>

	* solib-svr4.c (svr4_find_and_create_probe_breakpoints): Check all
	probe arguments.
---
 gdb/solib-svr4.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index 2a44dd6985..ffae26bfc5 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -2071,7 +2071,6 @@ svr4_find_and_create_probe_breakpoints (svr4_info *info,
 					bool with_prefix)
 {
   std::vector<probe *> probes[NUM_PROBES];
-  bool checked_can_use_probe_arguments = false;
 
   for (int i = 0; i < NUM_PROBES; i++)
     {
@@ -2102,12 +2101,23 @@ svr4_find_and_create_probe_breakpoints (svr4_info *info,
 	return false;
 
       /* Ensure probe arguments can be evaluated.  */
-      if (!checked_can_use_probe_arguments)
+      for (probe *p : probes[i])
 	{
-	  probe *p = probes[i][0];
 	  if (!p->can_evaluate_arguments ())
 	    return false;
-	  checked_can_use_probe_arguments = true;
+	  /* This will fail if the probe is invalid.  This has been seen on Arm
+	     due to references to symbols that have been resolved away.  */
+	  try
+	    {
+	      p->get_argument_count (gdbarch);
+	    }
+	  catch (const gdb_exception_error &ex)
+	    {
+	      exception_print (gdb_stderr, ex);
+	      warning (_("Initializing probes-based dynamic linker interface "
+			 "failed.\nReverting to original interface."));
+	      return false;
+	    }
 	}
     }
 
-- 
2.20.1 (Apple Git-117)

[PATCH v2 2/3] Use gdbarch for probe::get_argument_count

[Alan Hayward]

The probe function get_argument_count does not need a frame, only
the current gdbarch.  Switch the code to pass gdbarch instead.
No functional changes.

gdb/ChangeLog:

2019-08-21  Alan Hayward  <alan.hayward@arm.com>

	* break-catch-throw.c (fetch_probe_arguments): Use gdbarch.
	* dtrace-probe.c (dtrace_probe::get_argument_count): Likewise.
	* probe.c (probe_safe_evaluate_at_pc) (compute_probe_arg)
	(compile_probe_arg): Likewise.
	* probe.h (get_argument_count): Likewise.
	* solib-svr4.c (solib_event_probe_action): Likewise.
	* stap-probe.c (stap_probe::get_argument_count): Likewise.
---
 gdb/break-catch-throw.c | 2 +-
 gdb/dtrace-probe.c      | 4 ++--
 gdb/probe.c             | 7 +++----
 gdb/probe.h             | 2 +-
 gdb/solib-svr4.c        | 2 +-
 gdb/stap-probe.c        | 6 ++----
 6 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index 0677a55ee5..2c2a3b7d72 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -113,7 +113,7 @@ fetch_probe_arguments (struct value **arg0, struct value **arg1)
 	  && pc_probe.prob->get_name () != "rethrow"))
     error (_("not stopped at a C++ exception catchpoint"));
 
-  n_args = pc_probe.prob->get_argument_count (frame);
+  n_args = pc_probe.prob->get_argument_count (get_frame_arch (frame));
   if (n_args < 2)
     error (_("C++ exception catchpoint has too few arguments"));
 
diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
index 2d92edb11c..e9e71fd4c9 100644
--- a/gdb/dtrace-probe.c
+++ b/gdb/dtrace-probe.c
@@ -122,7 +122,7 @@ public:
   CORE_ADDR get_relocated_address (struct objfile *objfile) override;
 
   /* See probe.h.  */
-  unsigned get_argument_count (struct frame_info *frame) override;
+  unsigned get_argument_count (struct gdbarch *gdbarch) override;
 
   /* See probe.h.  */
   bool can_evaluate_arguments () const override;
@@ -693,7 +693,7 @@ dtrace_probe::get_relocated_address (struct objfile *objfile)
 /* Implementation of the get_argument_count method.  */
 
 unsigned
-dtrace_probe::get_argument_count (struct frame_info *frame)
+dtrace_probe::get_argument_count (struct gdbarch *gdbarch)
 {
   return m_args.size ();
 }
diff --git a/gdb/probe.c b/gdb/probe.c
index cdc6e021d2..8b108d6b02 100644
--- a/gdb/probe.c
+++ b/gdb/probe.c
@@ -695,7 +695,7 @@ probe_safe_evaluate_at_pc (struct frame_info *frame, unsigned n)
   if (!probe.prob)
     return NULL;
 
-  n_args = probe.prob->get_argument_count (frame);
+  n_args = probe.prob->get_argument_count (get_frame_arch (frame));
   if (n >= n_args)
     return NULL;
 
@@ -818,7 +818,7 @@ compute_probe_arg (struct gdbarch *arch, struct internalvar *ivar,
   if (pc_probe.prob == NULL)
     error (_("No probe at PC %s"), core_addr_to_string (pc));
 
-  n_args = pc_probe.prob->get_argument_count (frame);
+  n_args = pc_probe.prob->get_argument_count (arch);
   if (sel == -1)
     return value_from_longest (builtin_type (arch)->builtin_int, n_args);
 
@@ -840,7 +840,6 @@ compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
   int sel = (int) (uintptr_t) data;
   struct bound_probe pc_probe;
   int n_args;
-  struct frame_info *frame = get_selected_frame (NULL);
 
   /* SEL == -1 means "_probe_argc".  */
   gdb_assert (sel >= -1);
@@ -849,7 +848,7 @@ compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
   if (pc_probe.prob == NULL)
     error (_("No probe at PC %s"), core_addr_to_string (pc));
 
-  n_args = pc_probe.prob->get_argument_count (frame);
+  n_args = pc_probe.prob->get_argument_count (expr->gdbarch);
 
   if (sel == -1)
     {
diff --git a/gdb/probe.h b/gdb/probe.h
index 8abf69e354..7410c5aadf 100644
--- a/gdb/probe.h
+++ b/gdb/probe.h
@@ -131,7 +131,7 @@ public:
 
   /* Return the number of arguments of the probe.  This function can
      throw an exception.  */
-  virtual unsigned get_argument_count (struct frame_info *frame) = 0;
+  virtual unsigned get_argument_count (struct gdbarch *gdbarch) = 0;
 
   /* Return 1 if the probe interface can evaluate the arguments of
      probe, zero otherwise.  See the comments on
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index b21eacb68f..2a44dd6985 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -1756,7 +1756,7 @@ solib_event_probe_action (struct probe_and_action *pa)
        arg2: struct link_map *new (optional, for incremental updates)  */
   try
     {
-      probe_argc = pa->prob->get_argument_count (frame);
+      probe_argc = pa->prob->get_argument_count (get_frame_arch (frame));
     }
   catch (const gdb_exception_error &ex)
     {
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index b6de873d2c..700b657967 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -136,7 +136,7 @@ public:
   CORE_ADDR get_relocated_address (struct objfile *objfile) override;
 
   /* See probe.h.  */
-  unsigned get_argument_count (struct frame_info *frame) override;
+  unsigned get_argument_count (struct gdbarch *gdbarch) override;
 
   /* See probe.h.  */
   bool can_evaluate_arguments () const override;
@@ -1301,10 +1301,8 @@ stap_probe::get_relocated_address (struct objfile *objfile)
    argument string.  */
 
 unsigned
-stap_probe::get_argument_count (struct frame_info *frame)
+stap_probe::get_argument_count (struct gdbarch *gdbarch)
 {
-  struct gdbarch *gdbarch = get_frame_arch (frame);
-
   if (!m_have_parsed_args)
     {
       if (this->can_evaluate_arguments ())
-- 
2.20.1 (Apple Git-117)

Re: [PATCH v2 2/3] Use gdbarch for probe::get_argument_count

[Sergio Durigan Junior]

On Wednesday, August 21 2019, Alan Hayward wrote:

> The probe function get_argument_count does not need a frame, only
> the current gdbarch.  Switch the code to pass gdbarch instead.
> No functional changes.

The patch looks good at first glance, but I'd like to apply it before
approving.  Unfortunately, it seems like your MUA has mangled the
message (which is strange, because apparently you're using
git-send-email):

  <https://sourceware.org/cgi-bin/get-raw-msg?listname=gdb-patches&date=2019-08&msgid=20190821155816.45504-3-alan.hayward%40arm.com>

Can you resend it, please?

> gdb/ChangeLog:
>
> 2019-08-21  Alan Hayward  <alan.hayward@arm.com>
>
> 	* break-catch-throw.c (fetch_probe_arguments): Use gdbarch.
> 	* dtrace-probe.c (dtrace_probe::get_argument_count): Likewise.
> 	* probe.c (probe_safe_evaluate_at_pc) (compute_probe_arg)
> 	(compile_probe_arg): Likewise.
> 	* probe.h (get_argument_count): Likewise.
> 	* solib-svr4.c (solib_event_probe_action): Likewise.
> 	* stap-probe.c (stap_probe::get_argument_count): Likewise.
> ---
>  gdb/break-catch-throw.c | 2 +-
>  gdb/dtrace-probe.c      | 4 ++--
>  gdb/probe.c             | 7 +++----
>  gdb/probe.h             | 2 +-
>  gdb/solib-svr4.c        | 2 +-
>  gdb/stap-probe.c        | 6 ++----
>  6 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
> index 0677a55ee5..2c2a3b7d72 100644
> --- a/gdb/break-catch-throw.c
> +++ b/gdb/break-catch-throw.c
> @@ -113,7 +113,7 @@ fetch_probe_arguments (struct value **arg0, struct value **arg1)
>  	  && pc_probe.prob->get_name () != "rethrow"))
>      error (_("not stopped at a C++ exception catchpoint"));
>  
> -  n_args = pc_probe.prob->get_argument_count (frame);
> +  n_args = pc_probe.prob->get_argument_count (get_frame_arch (frame));
>    if (n_args < 2)
>      error (_("C++ exception catchpoint has too few arguments"));
>  
> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c
> index 2d92edb11c..e9e71fd4c9 100644
> --- a/gdb/dtrace-probe.c
> +++ b/gdb/dtrace-probe.c
> @@ -122,7 +122,7 @@ public:
>    CORE_ADDR get_relocated_address (struct objfile *objfile) override;
>  
>    /* See probe.h.  */
> -  unsigned get_argument_count (struct frame_info *frame) override;
> +  unsigned get_argument_count (struct gdbarch *gdbarch) override;
>  
>    /* See probe.h.  */
>    bool can_evaluate_arguments () const override;
> @@ -693,7 +693,7 @@ dtrace_probe::get_relocated_address (struct objfile *objfile)
>  /* Implementation of the get_argument_count method.  */
>  
>  unsigned
> -dtrace_probe::get_argument_count (struct frame_info *frame)
> +dtrace_probe::get_argument_count (struct gdbarch *gdbarch)
>  {
>    return m_args.size ();
>  }
> diff --git a/gdb/probe.c b/gdb/probe.c
> index cdc6e021d2..8b108d6b02 100644
> --- a/gdb/probe.c
> +++ b/gdb/probe.c
> @@ -695,7 +695,7 @@ probe_safe_evaluate_at_pc (struct frame_info *frame, unsigned n)
>    if (!probe.prob)
>      return NULL;
>  
> -  n_args = probe.prob->get_argument_count (frame);
> +  n_args = probe.prob->get_argument_count (get_frame_arch (frame));
>    if (n >= n_args)
>      return NULL;
>  
> @@ -818,7 +818,7 @@ compute_probe_arg (struct gdbarch *arch, struct internalvar *ivar,
>    if (pc_probe.prob == NULL)
>      error (_("No probe at PC %s"), core_addr_to_string (pc));
>  
> -  n_args = pc_probe.prob->get_argument_count (frame);
> +  n_args = pc_probe.prob->get_argument_count (arch);
>    if (sel == -1)
>      return value_from_longest (builtin_type (arch)->builtin_int, n_args);
>  
> @@ -840,7 +840,6 @@ compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
>    int sel = (int) (uintptr_t) data;
>    struct bound_probe pc_probe;
>    int n_args;
> -  struct frame_info *frame = get_selected_frame (NULL);
>  
>    /* SEL == -1 means "_probe_argc".  */
>    gdb_assert (sel >= -1);
> @@ -849,7 +848,7 @@ compile_probe_arg (struct internalvar *ivar, struct agent_expr *expr,
>    if (pc_probe.prob == NULL)
>      error (_("No probe at PC %s"), core_addr_to_string (pc));
>  
> -  n_args = pc_probe.prob->get_argument_count (frame);
> +  n_args = pc_probe.prob->get_argument_count (expr->gdbarch);
>  
>    if (sel == -1)
>      {
> diff --git a/gdb/probe.h b/gdb/probe.h
> index 8abf69e354..7410c5aadf 100644
> --- a/gdb/probe.h
> +++ b/gdb/probe.h
> @@ -131,7 +131,7 @@ public:
>  
>    /* Return the number of arguments of the probe.  This function can
>       throw an exception.  */
> -  virtual unsigned get_argument_count (struct frame_info *frame) = 0;
> +  virtual unsigned get_argument_count (struct gdbarch *gdbarch) = 0;
>  
>    /* Return 1 if the probe interface can evaluate the arguments of
>       probe, zero otherwise.  See the comments on
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index b21eacb68f..2a44dd6985 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -1756,7 +1756,7 @@ solib_event_probe_action (struct probe_and_action *pa)
>         arg2: struct link_map *new (optional, for incremental updates)  */
>    try
>      {
> -      probe_argc = pa->prob->get_argument_count (frame);
> +      probe_argc = pa->prob->get_argument_count (get_frame_arch (frame));
>      }
>    catch (const gdb_exception_error &ex)
>      {
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index b6de873d2c..700b657967 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -136,7 +136,7 @@ public:
>    CORE_ADDR get_relocated_address (struct objfile *objfile) override;
>  
>    /* See probe.h.  */
> -  unsigned get_argument_count (struct frame_info *frame) override;
> +  unsigned get_argument_count (struct gdbarch *gdbarch) override;
>  
>    /* See probe.h.  */
>    bool can_evaluate_arguments () const override;
> @@ -1301,10 +1301,8 @@ stap_probe::get_relocated_address (struct objfile *objfile)
>     argument string.  */
>  
>  unsigned
> -stap_probe::get_argument_count (struct frame_info *frame)
> +stap_probe::get_argument_count (struct gdbarch *gdbarch)
>  {
> -  struct gdbarch *gdbarch = get_frame_arch (frame);
> -
>    if (!m_have_parsed_args)
>      {
>        if (this->can_evaluate_arguments ())
> -- 
> 2.20.1 (Apple Git-117)

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH v2 2/3] Use gdbarch for probe::get_argument_count

[Sergio Durigan Junior]

On Wednesday, September 04 2019, I wrote:

> On Wednesday, August 21 2019, Alan Hayward wrote:
>
>> The probe function get_argument_count does not need a frame, only
>> the current gdbarch.  Switch the code to pass gdbarch instead.
>> No functional changes.
>
> The patch looks good at first glance, but I'd like to apply it before
> approving.  Unfortunately, it seems like your MUA has mangled the
> message (which is strange, because apparently you're using
> git-send-email):
>
>   <https://sourceware.org/cgi-bin/get-raw-msg?listname=gdb-patches&date=2019-08&msgid=20190821155816.45504-3-alan.hayward%40arm.com>
>
> Can you resend it, please?

Ah, I see that the whole patch series has already been pushed.  Sorry,
and please disregard my message.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH v2 0/3] Fix stop-on-solib event failures

[Gary Benson]

Hi Alan,

Alan Hayward wrote:
> On some Arm targets (namely the buildbot Arm Docker setup) placing
> breakpoints on just the solib dynamic probes will cause the target
> process to not stop.  This is due to the probes being invalid - see
> link in 3/3 for more details.
> 
> Fix is to fully validate the probes before using the,.
> 
> Patches 1 and 2 are code refactors. The actual fix is in patch 3.

The code looks good to me, my only caveat being that I think we're
supposed to wrap lines at 72 columns (unless that changed) and some
lines in your patches seem too long.  I noticed it in comments, but
possibly it's in code too.  With that fixed (or, not fixed if we
don't wrap at 72 columns any more) I'd say this is good to commit.
Thank you for doing the work!

Cheers,
Gary

Re: [PATCH v2 0/3] Fix stop-on-solib event failures

[Alan Hayward]

> On 30 Aug 2019, at 16:51, Gary Benson <gbenson@redhat.com> wrote:
> 
> Hi Alan,
> 
> Alan Hayward wrote:
>> On some Arm targets (namely the buildbot Arm Docker setup) placing
>> breakpoints on just the solib dynamic probes will cause the target
>> process to not stop.  This is due to the probes being invalid - see
>> link in 3/3 for more details.
>> 
>> Fix is to fully validate the probes before using the,.
>> 
>> Patches 1 and 2 are code refactors. The actual fix is in patch 3.
> 
> The code looks good to me, my only caveat being that I think we're
> supposed to wrap lines at 72 columns (unless that changed) and some
> lines in your patches seem too long.  I noticed it in comments, but
> possibly it's in code too.  With that fixed (or, not fixed if we
> don't wrap at 72 columns any more) I'd say this is good to commit.
> Thank you for doing the work!
> 

I’ve always kept within 80. Double checking the coding standards, it has:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Column_limits
I’ve always wondered why there’s a lot of code that wraps earlier and
that explains why. (personally I’d prefer even longer, but that’s just me).

Anyway, pushed patch to head.
Thanks!

Alan.