Re: Fix execl.exp sporadic failures

Pedro Alves
2008-07-08T11:05:33Z

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