Re: Fix execl.exp sporadic failures

[Pedro Alves <pedro@codesourcery.com>]

[-- 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: