Fix execl.exp sporadic failures

Fix execl.exp sporadic failures

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]

Sporadically the new execl.exp test fails with:

Executing new program: /home/pedro/gdb/execl_hunt/build32/gdb/testsuite/gdb.threads/execl1
Error in re-setting breakpoint 2: No source file named ../../../src/gdb/testsuite/gdb.threads/execl.c.
warning: Cannot initialize thread debugging library: versions of libpthread and libthread_db do not match
/home/pedro/gdb/execl_hunt/build32/gdb/testsuite/gdb.threads/execl1: error while loading shared libraries: libc.so.6:
failed to map segment from shared object: Cannot allocate memory

Program exited with code 0177.
(gdb) FAIL: gdb.threads/execl.exp: continue across exec (the program exited)

I traced to problem into the fact that when thread_db is loaded, GDB
will delete a breakpoint that was inserted in the old image from the
new image, which is a bad thing.  The breakpoint shadows of the old
image are meaningless for the new image.

There's a call to mark_breakpoints_out in update_breakpoints_after_exec, which is
called by follow_exec, which in turn is called by handle_inferior_event on
a TARGET_WAITKIND_EXECD, that takes care of marking all breakpoints as not
inserted, to present this case.

The problem starts here:

static ptid_t
thread_db_wait (ptid_t ptid, struct target_waitstatus *ourstatus)
{
  ptid = target_beneath->to_wait (ptid, ourstatus);

  (...)

  if (ourstatus->kind == TARGET_WAITKIND_EXECD)
    {
      remove_thread_event_breakpoints (); <<<<<
      unpush_target (&thread_db_ops);
      using_thread_db = 0;

      return ptid;
    }

remove_thread_event_breakpoints calls delete_breakpoint,
and this happens before reaching handle_inferior_event, hence at this
point, the breakpoints that were inserted in the old image are still
mark as such, which triggers the breakpoint shadows to be inserted
into the new exec image.

It is my opition, that defering the mark_breakpoints_out call to so
late out of the target is not safe.  It is better to calls it as
soon as we detect that the inferior execed.

That is what the attached patch does.

Tested on x86_64-unknown-linux-gnu.  I no longer get sporadic
failures on execl.exp.  They were much easier to trigger with
-m32 for some reason).

breakpoints always-inserted mode requires yet another fix,
which I'll post in a sec.

-- 
Pedro Alves

[-- Attachment #2: mark_breakpoints_out_sooner.diff --]
[-- Type: text/x-diff, Size: 4502 bytes --]

2008-07-03  Pedro Alves  <pedro@codesourcery.com>

	* breakpoint.c (mark_breakpoints_out): Make public.
	(update_breakpoints_after_exec): Don't call mark_breakpoints_out
	here.  Update comment.
	* breakpoint.h (mark_breakpoints_out): Declare.

	* linux-nat.c (linux_handle_extended_wait): On
	TARGET_WAITKIND_EXECD, call mark_breakpoints_out.
	* inf-ttrace.c (inf_ttrace_wait): Likewise.

---
 gdb/breakpoint.c |   21 ++++++++++++++-------
 gdb/breakpoint.h |    3 +++
 gdb/inf-ttrace.c |    6 ++++++
 gdb/linux-nat.c  |   10 ++++++++++
 4 files changed, 33 insertions(+), 7 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2008-07-03 01:24:21.000000000 +0100
+++ src/gdb/breakpoint.c	2008-07-03 01:41:11.000000000 +0100
@@ -188,8 +188,6 @@ static int single_step_breakpoint_insert
 
 static void free_bp_location (struct bp_location *loc);
 
-static void mark_breakpoints_out (void);
-
 static struct bp_location *
 allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
 
@@ -1445,10 +1443,19 @@ update_breakpoints_after_exec (void)
   struct breakpoint *temp;
   struct cleanup *cleanup;
 
-  /* Doing this first prevents the badness of having delete_breakpoint()
-     write a breakpoint's current "shadow contents" to lift the bp.  That
-     shadow is NOT valid after an exec()! */
-  mark_breakpoints_out ();
+  /* There used to be a call to mark_breakpoints_out here with the
+     following comment:
+
+      Doing this first prevents the badness of having
+      delete_breakpoint() write a breakpoint's current "shadow
+      contents" to lift the bp.  That shadow is NOT valid after an
+      exec()!
+
+     The concern is valid, but it was found that there are logical
+     places to delete breakpoints after detecting an exec and before
+     reaching here.  The call has since moved closer to where the each
+     target detects an exec.  */
+
 
   /* The binary we used to debug is now gone, and we're updating
      breakpoints for the new binary.  Until we're done, we should not
@@ -1699,7 +1706,7 @@ remove_breakpoint (struct bp_location *b
 
 /* Clear the "inserted" flag in all breakpoints.  */
 
-static void
+void
 mark_breakpoints_out (void)
 {
   struct bp_location *bpt;
Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h	2008-07-03 01:24:21.000000000 +0100
+++ src/gdb/breakpoint.h	2008-07-03 01:40:14.000000000 +0100
@@ -826,6 +826,9 @@ extern void disable_breakpoint (struct b
 
 extern void enable_breakpoint (struct breakpoint *);
 
+/* Clear the "inserted" flag in all breakpoints.  */
+extern void mark_breakpoints_out (void);
+
 extern void make_breakpoint_permanent (struct breakpoint *);
 
 extern struct breakpoint *create_solib_event_breakpoint (CORE_ADDR);
Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2008-07-03 01:24:21.000000000 +0100
+++ src/gdb/linux-nat.c	2008-07-03 01:40:14.000000000 +0100
@@ -1758,6 +1758,16 @@ linux_handle_extended_wait (struct lwp_i
 	  linux_parent_pid = 0;
 	}
 
+      /* At this point, all inserted breakpoints are gone.  Doing this
+	 as soon as we detect an exec prevents the badness of deleting
+	 a breakpoint writing the current "shadow contents" to lift
+	 the bp.  That shadow is NOT valid after an exec.
+
+	 Note that we have to do this after the detach_breakpoints
+	 call above, otherwise breakpoints wouldn't be lifted from the
+	 parent on a vfork, because detach_breakpoints would think
+	 that breakpoints are not inserted.  */
+      mark_breakpoints_out ();
       return 0;
     }
 
Index: src/gdb/inf-ttrace.c
===================================================================
--- src.orig/gdb/inf-ttrace.c	2008-07-03 01:24:21.000000000 +0100
+++ src/gdb/inf-ttrace.c	2008-07-03 01:40:14.000000000 +0100
@@ -904,6 +904,12 @@ inf_ttrace_wait (ptid_t ptid, struct tar
 		  tts.tts_u.tts_exec.tts_pathlen, 0) == -1)
 	perror_with_name (("ttrace"));
       ourstatus->value.execd_pathname[tts.tts_u.tts_exec.tts_pathlen] = 0;
+
+      /* At this point, all inserted breakpoints are gone.  Doing this
+	 as soon as we detect an exec prevents the badness of deleting
+	 a breakpoint writing the current "shadow contents" to lift
+	 the bp.  That shadow is NOT valid after an exec.  */
+      mark_breakpoints_out ();
       break;
 
     case TTEVT_EXIT:

Re: Fix execl.exp sporadic failures

[Daniel Jacobowitz]

On Thu, Jul 03, 2008 at 01:43:08AM +0100, Pedro Alves wrote:
> 2008-07-03  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* breakpoint.c (mark_breakpoints_out): Make public.
> 	(update_breakpoints_after_exec): Don't call mark_breakpoints_out
> 	here.  Update comment.
> 	* breakpoint.h (mark_breakpoints_out): Declare.
> 
> 	* linux-nat.c (linux_handle_extended_wait): On
> 	TARGET_WAITKIND_EXECD, call mark_breakpoints_out.
> 	* inf-ttrace.c (inf_ttrace_wait): Likewise.

I don't like this patch - it duplicates knowledge into each GDB
target that should be global - but your logic is sound.  It's OK.
> +  /* There used to be a call to mark_breakpoints_out here with the
> +     following comment:
> +
> +      Doing this first prevents the badness of having
> +      delete_breakpoint() write a breakpoint's current "shadow
> +      contents" to lift the bp.  That shadow is NOT valid after an
> +      exec()!
> +
> +     The concern is valid, but it was found that there are logical
> +     places to delete breakpoints after detecting an exec and before
> +     reaching here.  The call has since moved closer to where the each
> +     target detects an exec.  */
> +

Please remove this comment, or write one that describes the current
state (bonus points for an assertion).  Comments that describe how GDB
used to be grow more confusing with their age.

>    /* The binary we used to debug is now gone, and we're updating
>       breakpoints for the new binary.  Until we're done, we should not

This comment is now out of date, but it's removed by the other patch I
just approved, very nice :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix execl.exp sporadic failures

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On Monday 07 July 2008 20:10:36, Daniel Jacobowitz wrote:
> On Thu, Jul 03, 2008 at 01:43:08AM +0100, Pedro Alves wrote:

> It's OK. 

> On Thu, Jul 03, 2008 at 01:43:08AM +0100, Pedro Alves wrote:
> > +  /* There used to be a call to mark_breakpoints_out here with the
> > +     following comment:
> > +
> > +      Doing this first prevents the badness of having
> > +      delete_breakpoint() write a breakpoint's current "shadow
> > +      contents" to lift the bp.  That shadow is NOT valid after an
> > +      exec()!
> > +
> > +     The concern is valid, but it was found that there are logical
> > +     places to delete breakpoints after detecting an exec and before
> > +     reaching here.  The call has since moved closer to where the each
> > +     target detects an exec.  */
> > +
>
> Please remove this comment, or write one that describes the current
> state (bonus points for an assertion).  Comments that describe how GDB
> used to be grow more confusing with their age.

/me wants bonus points.

Attached is what I checked in then.

Thanks!

-- 
Pedro Alves

[-- Attachment #2: mark_breakpoints_out_sooner.diff --]
[-- Type: text/x-diff, Size: 4502 bytes --]

2008-07-08  Pedro Alves  <pedro@codesourcery.com>

	* breakpoint.c (mark_breakpoints_out): Make public.
	(update_breakpoints_after_exec): Don't call mark_breakpoints_out
	here.  Update comment.
	* breakpoint.h (mark_breakpoints_out): Declare.

	* linux-nat.c (linux_handle_extended_wait): On
	TARGET_WAITKIND_EXECD, call mark_breakpoints_out.
	* inf-ttrace.c (inf_ttrace_wait): Likewise.

---
 gdb/breakpoint.c |   21 ++++++++++++++-------
 gdb/breakpoint.h |    3 +++
 gdb/inf-ttrace.c |    6 ++++++
 gdb/linux-nat.c  |   10 ++++++++++
 4 files changed, 33 insertions(+), 7 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2008-07-03 01:24:21.000000000 +0100
+++ src/gdb/breakpoint.c	2008-07-03 01:41:11.000000000 +0100
@@ -188,8 +188,6 @@ static int single_step_breakpoint_insert
 
 static void free_bp_location (struct bp_location *loc);
 
-static void mark_breakpoints_out (void);
-
 static struct bp_location *
 allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
 
@@ -1445,10 +1443,19 @@ update_breakpoints_after_exec (void)
   struct breakpoint *temp;
   struct cleanup *cleanup;
 
-  /* Doing this first prevents the badness of having delete_breakpoint()
-     write a breakpoint's current "shadow contents" to lift the bp.  That
-     shadow is NOT valid after an exec()! */
-  mark_breakpoints_out ();
+  /* There used to be a call to mark_breakpoints_out here with the
+     following comment:
+
+      Doing this first prevents the badness of having
+      delete_breakpoint() write a breakpoint's current "shadow
+      contents" to lift the bp.  That shadow is NOT valid after an
+      exec()!
+
+     The concern is valid, but it was found that there are logical
+     places to delete breakpoints after detecting an exec and before
+     reaching here.  The call has since moved closer to where the each
+     target detects an exec.  */
+
 
   /* The binary we used to debug is now gone, and we're updating
      breakpoints for the new binary.  Until we're done, we should not
@@ -1699,7 +1706,7 @@ remove_breakpoint (struct bp_location *b
 
 /* Clear the "inserted" flag in all breakpoints.  */
 
-static void
+void
 mark_breakpoints_out (void)
 {
   struct bp_location *bpt;
Index: src/gdb/breakpoint.h
===================================================================
--- src.orig/gdb/breakpoint.h	2008-07-03 01:24:21.000000000 +0100
+++ src/gdb/breakpoint.h	2008-07-03 01:40:14.000000000 +0100
@@ -826,6 +826,9 @@ extern void disable_breakpoint (struct b
 
 extern void enable_breakpoint (struct breakpoint *);
 
+/* Clear the "inserted" flag in all breakpoints.  */
+extern void mark_breakpoints_out (void);
+
 extern void make_breakpoint_permanent (struct breakpoint *);
 
 extern struct breakpoint *create_solib_event_breakpoint (CORE_ADDR);
Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2008-07-03 01:24:21.000000000 +0100
+++ src/gdb/linux-nat.c	2008-07-03 01:40:14.000000000 +0100
@@ -1758,6 +1758,16 @@ linux_handle_extended_wait (struct lwp_i
 	  linux_parent_pid = 0;
 	}
 
+      /* At this point, all inserted breakpoints are gone.  Doing this
+	 as soon as we detect an exec prevents the badness of deleting
+	 a breakpoint writing the current "shadow contents" to lift
+	 the bp.  That shadow is NOT valid after an exec.
+
+	 Note that we have to do this after the detach_breakpoints
+	 call above, otherwise breakpoints wouldn't be lifted from the
+	 parent on a vfork, because detach_breakpoints would think
+	 that breakpoints are not inserted.  */
+      mark_breakpoints_out ();
       return 0;
     }
 
Index: src/gdb/inf-ttrace.c
===================================================================
--- src.orig/gdb/inf-ttrace.c	2008-07-03 01:24:21.000000000 +0100
+++ src/gdb/inf-ttrace.c	2008-07-03 01:40:14.000000000 +0100
@@ -904,6 +904,12 @@ inf_ttrace_wait (ptid_t ptid, struct tar
 		  tts.tts_u.tts_exec.tts_pathlen, 0) == -1)
 	perror_with_name (("ttrace"));
       ourstatus->value.execd_pathname[tts.tts_u.tts_exec.tts_pathlen] = 0;
+
+      /* At this point, all inserted breakpoints are gone.  Doing this
+	 as soon as we detect an exec prevents the badness of deleting
+	 a breakpoint writing the current "shadow contents" to lift
+	 the bp.  That shadow is NOT valid after an exec.  */
+      mark_breakpoints_out ();
       break;
 
     case TTEVT_EXIT:

Re: Fix execl.exp sporadic failures

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 158 bytes --]

A Tuesday 08 July 2008 12:05:05, Pedro Alves wrote:
> Attached is what I checked in then.

Sorry, EWRONGPATCH.  *This* is what I checked in.

-- 
Pedro Alves

[-- Attachment #2: execl.diff --]
[-- Type: text/x-diff, Size: 5221 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.9522
retrieving revision 1.9523
diff -u -p -r1.9522 -r1.9523
--- ChangeLog	8 Jul 2008 10:31:15 -0000	1.9522
+++ ChangeLog	8 Jul 2008 10:59:57 -0000	1.9523
@@ -1,5 +1,16 @@
 2008-07-08  Pedro Alves  <pedro@codesourcery.com>
 
+	* breakpoint.c (mark_breakpoints_out): Make public.
+	(update_breakpoints_after_exec): Don't call mark_breakpoints_out
+	here.  Update comment.
+	* breakpoint.h (mark_breakpoints_out): Declare.
+
+	* linux-nat.c (linux_handle_extended_wait): On
+	TARGET_WAITKIND_EXECD, call mark_breakpoints_out.
+	* inf-ttrace.c (inf_ttrace_wait): Likewise.
+
+2008-07-08  Pedro Alves  <pedro@codesourcery.com>
+
 	* infrun.c (follow_exec): Reset shared libraries before adding the
 	main exec file.
 
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.328
retrieving revision 1.329
diff -u -p -r1.328 -r1.329
--- breakpoint.c	7 Jul 2008 22:39:58 -0000	1.328
+++ breakpoint.c	8 Jul 2008 10:59:57 -0000	1.329
@@ -188,8 +188,6 @@ static int single_step_breakpoint_insert
 
 static void free_bp_location (struct bp_location *loc);
 
-static void mark_breakpoints_out (void);
-
 static struct bp_location *
 allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
 
@@ -1443,12 +1441,19 @@ update_breakpoints_after_exec (void)
 {
   struct breakpoint *b;
   struct breakpoint *temp;
+  struct bp_location *bploc;
   struct cleanup *cleanup;
 
-  /* Doing this first prevents the badness of having delete_breakpoint()
-     write a breakpoint's current "shadow contents" to lift the bp.  That
-     shadow is NOT valid after an exec()! */
-  mark_breakpoints_out ();
+  /* We're about to delete breakpoints from GDB's lists.  If the
+     INSERTED flag is true, GDB will try to lift the breakpoints by
+     writing the breakpoints' "shadow contents" back into memory.  The
+     "shadow contents" are NOT valid after an exec, so GDB should not
+     do that.  Instead, the target is responsible from marking
+     breakpoints out as soon as it detects an exec.  We don't do that
+     here instead, because there may be other attempts to delete
+     breakpoints after detecting an exec and before reaching here.  */
+  ALL_BP_LOCATIONS (bploc)
+    gdb_assert (!bploc->inserted);
 
   /* The binary we used to debug is now gone, and we're updating
      breakpoints for the new binary.  Until we're done, we should not
@@ -1699,7 +1704,7 @@ remove_breakpoint (struct bp_location *b
 
 /* Clear the "inserted" flag in all breakpoints.  */
 
-static void
+void
 mark_breakpoints_out (void)
 {
   struct bp_location *bpt;
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.73
retrieving revision 1.74
diff -u -p -r1.73 -r1.74
--- breakpoint.h	28 Jun 2008 09:42:15 -0000	1.73
+++ breakpoint.h	8 Jul 2008 10:59:57 -0000	1.74
@@ -826,6 +826,9 @@ extern void disable_breakpoint (struct b
 
 extern void enable_breakpoint (struct breakpoint *);
 
+/* Clear the "inserted" flag in all breakpoints.  */
+extern void mark_breakpoints_out (void);
+
 extern void make_breakpoint_permanent (struct breakpoint *);
 
 extern struct breakpoint *create_solib_event_breakpoint (CORE_ADDR);
Index: inf-ttrace.c
===================================================================
RCS file: /cvs/src/src/gdb/inf-ttrace.c,v
retrieving revision 1.28
retrieving revision 1.29
diff -u -p -r1.28 -r1.29
--- inf-ttrace.c	21 Mar 2008 15:44:53 -0000	1.28
+++ inf-ttrace.c	8 Jul 2008 10:59:57 -0000	1.29
@@ -904,6 +904,12 @@ inf_ttrace_wait (ptid_t ptid, struct tar
 		  tts.tts_u.tts_exec.tts_pathlen, 0) == -1)
 	perror_with_name (("ttrace"));
       ourstatus->value.execd_pathname[tts.tts_u.tts_exec.tts_pathlen] = 0;
+
+      /* At this point, all inserted breakpoints are gone.  Doing this
+	 as soon as we detect an exec prevents the badness of deleting
+	 a breakpoint writing the current "shadow contents" to lift
+	 the bp.  That shadow is NOT valid after an exec.  */
+      mark_breakpoints_out ();
       break;
 
     case TTEVT_EXIT:
Index: linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.88
retrieving revision 1.89
diff -u -p -r1.88 -r1.89
--- linux-nat.c	28 Jun 2008 11:15:33 -0000	1.88
+++ linux-nat.c	8 Jul 2008 10:59:57 -0000	1.89
@@ -1758,6 +1758,16 @@ linux_handle_extended_wait (struct lwp_i
 	  linux_parent_pid = 0;
 	}
 
+      /* At this point, all inserted breakpoints are gone.  Doing this
+	 as soon as we detect an exec prevents the badness of deleting
+	 a breakpoint writing the current "shadow contents" to lift
+	 the bp.  That shadow is NOT valid after an exec.
+
+	 Note that we have to do this after the detach_breakpoints
+	 call above, otherwise breakpoints wouldn't be lifted from the
+	 parent on a vfork, because detach_breakpoints would think
+	 that breakpoints are not inserted.  */
+      mark_breakpoints_out ();
       return 0;
     }
 

Re: Fix execl.exp sporadic failures

[Stan Shebs]

Pedro Alves wrote:
> A Tuesday 08 July 2008 12:05:05, Pedro Alves wrote:
>   
>> Attached is what I checked in then.
>>     
>
> Sorry, EWRONGPATCH.  *This* is what I checked in.
>   
Bummer, used up your bonus points already.

:-)

Stan