Re: RFC: don't set the pspace on ordinary breakpoints

Tom Tromey
2011-11-09T18:24:19Z

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> I *think* the should_be_inserted change is all that was really needed,
Tom> after re-reading all the messages in this thread. Let me know what you
Tom> think.

Here is a refresh of this patch. This one is relative to CVS HEAD.
I didn't attempt to test it in isolation.

Tom

2011-10-28 Tom Tromey <tromey@redhat.com>

* elfread.c (elf_gnu_ifunc_resolver_return_stop): Allow
breakpoint's pspace to be NULL.
* breakpoint.h (enum enable_state) <bp_startup_disabled>: Remove.
(struct breakpoint) <pspace>: Update comment.
* breakpoint.c (should_be_inserted): Explicitly check if program
space is executing startup.
(describe_other_breakpoints): Update.
(init_raw_breakpoint): Conditionally set breakpoint's pspace.
(disable_breakpoints_before_startup): Change executing_startup
earlier. Use location's pspace.
(enable_breakpoints_after_startup): Likewise.
(init_breakpoint_sal): Don't set breakpoint's pspace. Don't use
bp_startup_disabled.
(create_breakpoint): Conditionally set breakpoint's pspace. Don't
use bp_startup_disabled.
(update_global_location_list): Use should_be_inserted.
(bkpt_re_set): Update.
(prepare_re_set_context): Conditionally switch program space.

From 0315321273c5dad6ec98744439c72e1e00729be7 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tromey@redhat.com>
Date: Tue, 8 Nov 2011 13:50:12 -0700
Subject: [PATCH 1/2] remove bp_startup_disabled

---
gdb/ChangeLog | 21 ++++++++++++++
gdb/breakpoint.c | 77 ++++++++++++++++++++++-------------------------------
gdb/breakpoint.h | 12 ++------
gdb/elfread.c | 2 +-
4 files changed, 57 insertions(+), 55 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..8ab09ba 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1568,6 +1568,9 @@ should_be_inserted (struct bp_location *bl)
if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
return 0;

+ if (user_breakpoint_p (bl->owner) && bl->pspace->executing_startup)
+ return 0;
+
/* This is set for example, when we're attached to the parent of a
vfork, and have detached from the child. The child is running
free, and we expect it to do an exec or exit, at which point the
@@ -5313,8 +5316,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
printf_filtered (" (thread %d)", b->thread);
printf_filtered ("%s%s ",
((b->enable_state == bp_disabled
- || b->enable_state == bp_call_disabled
- || b->enable_state == bp_startup_disabled)
+ || b->enable_state == bp_call_disabled)
? " (disabled)"
: b->enable_state == bp_permanent
? " (permanent)"
@@ -5790,9 +5792,11 @@ init_raw_breakpoint (struct breakpoint *b, struct gdbarch *gdbarch,
b->loc->address = adjusted_address;
b->loc->pspace = sal.pspace;

- /* Store the program space that was used to set the breakpoint, for
- breakpoint resetting. */
- b->pspace = sal.pspace;
+ /* Store the program space that was used to set the breakpoint,
+ except for ordinary breakpoints, which are independent of the
+ program space. */
+ if (bptype != bp_breakpoint && bptype != bp_hardware_breakpoint)
+ b->pspace = sal.pspace;

if (sal.symtab == NULL)
b->source_file = NULL;
@@ -6933,48 +6937,48 @@ enable_watchpoints_after_interactive_call_stop (void)
void
disable_breakpoints_before_startup (void)
{
- struct breakpoint *b;
+ struct bp_location *loc, **locp_tmp;
int found = 0;

- ALL_BREAKPOINTS (b)
+ current_program_space->executing_startup = 1;
+
+ ALL_BP_LOCATIONS (loc, locp_tmp)
{
- if (b->pspace != current_program_space)
+ if (loc->pspace != current_program_space)
continue;

- if ((b->type == bp_breakpoint
- || b->type == bp_hardware_breakpoint)
- && breakpoint_enabled (b))
+ if ((loc->owner->type == bp_breakpoint
+ || loc->owner->type == bp_hardware_breakpoint)
+ && breakpoint_enabled (loc->owner))
{
- b->enable_state = bp_startup_disabled;
found = 1;
+ break;
}
}

if (found)
update_global_location_list (0);
-
- current_program_space->executing_startup = 1;
}

void
enable_breakpoints_after_startup (void)
{
- struct breakpoint *b;
+ struct bp_location *loc, **locp_tmp;
int found = 0;

current_program_space->executing_startup = 0;

- ALL_BREAKPOINTS (b)
+ ALL_BP_LOCATIONS (loc, locp_tmp)
{
- if (b->pspace != current_program_space)
+ if (loc->pspace != current_program_space)
continue;

- if ((b->type == bp_breakpoint
- || b->type == bp_hardware_breakpoint)
- && b->enable_state == bp_startup_disabled)
+ if ((loc->owner->type == bp_breakpoint
+ || loc->owner->type == bp_hardware_breakpoint)
+ && breakpoint_enabled (loc->owner))
{
- b->enable_state = bp_enabled;
found = 1;
+ break;
}
}

@@ -7215,7 +7219,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
b->ignore_count = ignore_count;
b->enable_state = enabled ? bp_enabled : bp_disabled;
b->disposition = disposition;
- b->pspace = sals.sals[0].pspace;

if (type == bp_static_tracepoint)
{
@@ -7256,11 +7259,6 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
"tracepoint marker to probe"));
}

- if (enabled && b->pspace->executing_startup
- && (b->type == bp_breakpoint
- || b->type == bp_hardware_breakpoint))
- b->enable_state = bp_startup_disabled;
-
loc = b->loc;
}
else
@@ -7980,14 +7978,10 @@ create_breakpoint (struct gdbarch *gdbarch,
b->disposition = tempflag ? disp_del : disp_donttouch;
b->condition_not_parsed = 1;
b->enable_state = enabled ? bp_enabled : bp_disabled;
- b->pspace = current_program_space;
+ if (type_wanted != bp_breakpoint && type_wanted != bp_hardware_breakpoint)
+ b->pspace = current_program_space;
b->py_bp_object = NULL;

- if (enabled && b->pspace->executing_startup
- && (b->type == bp_breakpoint
- || b->type == bp_hardware_breakpoint))
- b->enable_state = bp_startup_disabled;
-
if (!internal)
/* Do not mention breakpoints with a negative number,
but do notify observers. */
@@ -10607,13 +10601,8 @@ update_global_location_list (int should_insert)
struct breakpoint *b = loc->owner;
struct bp_location **loc_first_p;

- if (b->enable_state == bp_disabled
- || b->enable_state == bp_call_disabled
- || b->enable_state == bp_startup_disabled
- || !loc->enabled
- || loc->shlib_disabled
- || !breakpoint_address_is_meaningful (b)
- || is_tracepoint (b))
+ if (!should_be_inserted (loc)
+ || !breakpoint_address_is_meaningful (b))
continue;

/* Permanent breakpoint should always be inserted. */
@@ -10891,10 +10880,6 @@ static struct breakpoint_ops base_breakpoint_ops =
static void
bkpt_re_set (struct breakpoint *b)
{
- /* Do not attempt to re-set breakpoints disabled during startup. */
- if (b->enable_state == bp_startup_disabled)
- return;
-
/* FIXME: is this still reachable? */
if (b->addr_string == NULL)
{
@@ -11825,6 +11810,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found)
if (e.error == NOT_FOUND_ERROR
&& (b->condition_not_parsed
|| (b->loc && b->loc->shlib_disabled)
+ || (b->loc && b->loc->pspace->executing_startup)
|| b->enable_state == bp_disabled))
not_found_and_ok = 1;

@@ -11913,7 +11899,8 @@ prepare_re_set_context (struct breakpoint *b)

input_radix = b->input_radix;
cleanups = save_current_space_and_thread ();
- switch_to_program_space_and_thread (b->pspace);
+ if (b->pspace != NULL)
+ switch_to_program_space_and_thread (b->pspace);
set_language (b->language);

return cleanups;
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..94e324a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -186,14 +186,6 @@ enum enable_state
automatically enabled and reset when the
call "lands" (either completes, or stops
at another eventpoint). */
- bp_startup_disabled, /* The eventpoint has been disabled during
- inferior startup. This is necessary on
- some targets where the main executable
- will get relocated during startup, making
- breakpoint addresses invalid. The
- eventpoint will be automatically enabled
- and reset once inferior startup is
- complete. */
bp_permanent /* There is a breakpoint instruction
hard-wired into the target's code. Don't
try to write another breakpoint
@@ -571,7 +563,9 @@ struct breakpoint
equals this. */
struct frame_id frame_id;

- /* The program space used to set the breakpoint. */
+ /* The program space used to set the breakpoint. This is only set
+ for breakpoints which are specific to a program space; for
+ ordinary breakpoints this is NULL. */
struct program_space *pspace;

/* String we used to set the breakpoint (malloc'd). */
diff --git a/gdb/elfread.c b/gdb/elfread.c
index a309a2c..067c77f 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1032,7 +1032,7 @@ elf_gnu_ifunc_resolver_return_stop (struct breakpoint *b)
}
gdb_assert (b->type == bp_gnu_ifunc_resolver);

- gdb_assert (current_program_space == b->pspace);
+ gdb_assert (current_program_space == b->pspace || b->pspace == NULL);
elf_gnu_ifunc_record_cache (b->addr_string, resolved_pc);

sal = find_pc_line (resolved_pc, 0);
--
1.7.6.4