[PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
Hannes Domani
ssbssa@yahoo.de
Fri Oct 9 18:51:53 GMT 2020
Am Freitag, 9. Oktober 2020, 20:22:21 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:
> Hi Hannes,
>
> On 6/7/20 1:56 PM, Hannes Domani via Gdb-patches wrote:
> > Am Sonntag, 31. Mai 2020, 18:38:06 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:
> >
> >> On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:
> >>
> >>> This patch creates an internal breakpoint on the process entry point, which
> >>> when it is reached, resets all active hardware breakpoints, and continues
> >>> execution.
> >>
> >> Missing ChangeLog entry.
> >>
> >>> ---
> >>> gdb/windows-tdep.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 130 insertions(+)
> >>>
> >>> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> >>> index aa0adeba99..90e4794fc5 100644
> >>> --- a/gdb/windows-tdep.c
> >>> +++ b/gdb/windows-tdep.c
> >>> @@ -37,6 +37,7 @@
> >>> #include "coff/internal.h"
> >>> #include "libcoff.h"
> >>> #include "solist.h"
> >>> +#include "observable.h"
> >>>
> >>> #define CYGWIN_DLL_NAME "cygwin1.dll"
> >>>
> >>> @@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
> >>> return siginfo_type;
> >>> }
> >>>
> >>> +/* Windows-specific cached data. This is used by GDB for caching
> >>> + purposes for each inferior. This helps reduce the overhead of
> >>> + transfering data from a remote target to the local host. */
> >>> +struct windows_info
> >>> +{
> >>> + CORE_ADDR entry_point = 0;
> >>> +};
> >>> +
> >>> +/* Per-inferior data key. */
> >>> +static const struct inferior_key<windows_info> windows_inferior_data;
> >>
> >> This should be program_space_key / per-program_space data, instead of
> >> per-inferior data.
> >>
> >> You may want to take a look at the jit.c code, which is doing similar
> >> things.
> >
> > OK. I will change it to program_space_key, but why are there no observers
> > to invalidate the a program-space, like these for inferiors?:
> >
> > /* Observers used to invalidate the cache when needed. */
> > gdb::observers::inferior_exit.attach (invalidate_windows_cache_inf);
> > gdb::observers::inferior_appeared.attach (invalidate_windows_cache_inf);
> >
> > Are they not needed for some reason?
>
> These notifications are more about the event that happened than about
> invalidating the inferior. Some observers may use the notification
> for that, others for other things.
>
> Probably nobody found a need for matching program space notifications
> because so far the existing ones have covered all needs.
I see.
> >
> >>> +
> >>> +/* Frees whatever allocated space there is to be freed and sets INF's
> >>> + Windows cache data pointer to NULL. */
> >>> +
> >>> +static void
> >>> +invalidate_windows_cache_inf (struct inferior *inf)
> >>> +{
> >>> + windows_inferior_data.clear (inf);
> >>> +}
> >>> +
> >>> +/* Fetch the Windows cache info for INF. This function always returns a
> >>> + valid INFO pointer. */
> >>> +
> >>> +static struct windows_info *
> >>> +get_windows_inferior_data (void)
> >>
> >> Drop the (void), only old pre-C++ code has it. You can also drop
> >> redundant "struct" throughout if you like.
> >>
> >>> +{
> >>> + struct windows_info *info;
> >>> + struct inferior *inf = current_inferior ();
> >>> +
> >>> + info = windows_inferior_data.get (inf);
> >>> + if (info == NULL)
> >>> + info = windows_inferior_data.emplace (inf);
> >>> +
> >>> + return info;
> >>> +}
> >>> +
> >>> +/* Breakpoint on entry point where any active hardware breakpoints will
> >>> + be reset. */
> >>
> >> Please expand the comments, explaining why this is necessary
> >> in the first place.
> >>
> >>> +static struct breakpoint_ops entry_point_breakpoint_ops;
> >>> +
> >>> +/* Reset active hardware breakpoints. */
> >>> +
> >>> +static bool
> >>> +reset_hardware_breakpoints (struct breakpoint *b)
> >>> +{
> >>> + if (b->type != bp_hardware_breakpoint
> >>> + && b->type != bp_hardware_watchpoint
> >>> + && b->type != bp_read_watchpoint
> >>> + && b->type != bp_access_watchpoint)
> >>> + return false;
> >>
> >> This should instead look at locations and their bp_loc_type,
> >> looking for bp_loc_hardware_breakpoint / bp_loc_hardware_watchpoint.
> >> There are situations where the breakpoint is a software breakpoint,
> >> but GDB still inserts a hardware breakpoint, like e.g., due
> >> to "set breakpoint auto-hw".
> >>
> >>> +
> >>> + struct bp_location *loc;
> >>> + for (loc = b->loc; loc; loc = loc->next)
> >>> + if (loc->enabled && loc->pspace == current_program_space
> >>> + && b->ops->remove_location (loc, REMOVE_BREAKPOINT) == 0)
> >>> + b->ops->insert_location (loc);
> >>
> >> This is incorrect for not considering whether a
> >> breakpoint location was enabled but not inserted (e.g., the overall
> >> breakpoint was disabled), or whether a breakpoint location was
> >> a duplicate.
> >>
> >> You should instead look at loc->inserted.
> >>
> >>> +
> >>> + return false;
> >>> +}
> >>> +
> >>> +/* This breakpoint type should never stop, but when reached, reset
> >>> + the active hardware breakpoints. */
> >>
> >> hardware breakpoints and watchpoints.
> >>
> >>> +
> >>> +static void
> >>> +startup_breakpoint_check_status (bpstat bs)
> >>> +{
> >>> + /* Never stop. */
> >>> + bs->stop = 0;
> >>> +
> >>> + iterate_over_breakpoints (reset_hardware_breakpoints);
> >>> +}
> >>> +
> >>> +/* Update the breakpoint location to the current entry point. */
> >>> +
> >>> +static void
> >>> +startup_breakpoint_re_set (struct breakpoint *b)
> >>> +{
> >>
> >> This is called if/when the loaded executable changes, even
> >> without re-starting an inferior. E.g., if you use the
> >> "file" command after starting the inferior. So this
> >> should re-fetch the new entry point from the executable.
> >> Again, take a look at the jit.c code.
> >
> > If I do "file" after "start", then windows_solib_create_inferior_hook is
> > called before startup_breakpoint_re_set, so the new entry point was
> > fetched already.
> >
> >
> >>> + struct windows_info *info = get_windows_inferior_data ();
> >>> + CORE_ADDR entry_point = info->entry_point;
> >>> +
> >>> + /* Do nothing if the entry point didn't change. */
> >>> + struct bp_location *loc;
> >>> + for (loc = b->loc; loc; loc = loc->next)
> >>> + if (loc->pspace == current_program_space && loc->address == entry_point)
> >>> + return;
> >>> +
> >>> + event_location_up location
> >>> + = new_address_location (entry_point, nullptr, 0);
> >>> + std::vector<symtab_and_line> sals;
> >>> + sals = b->ops->decode_location (b, location.get (), current_program_space);
> >>
> >> Merge the two statements, so that you end up copy initialization, instead of
> >> initialization, and then assignment:
> >>
> >> std::vector<symtab_and_line> sals
> >> = b->ops->decode_location (b, location.get (), current_program_space);
> >>
> >>> + update_breakpoint_locations (b, current_program_space, sals, {});
> >>> +}
> >>> +
> >>> /* Implement the "solib_create_inferior_hook" target_so_ops method. */
> >>>
> >>> static void
> >>> @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty)
> >>> if (vmaddr != exec_base)
> >>> objfile_rebase (symfile_objfile, exec_base - vmaddr);
> >>> }
> >>> +
> >>> + /* Create the entry point breakpoint if it doesn't exist already. */
> >>> + if (target_has_execution && exec_base != 0)
> >>> + {
> >>> + struct windows_info *info = get_windows_inferior_data ();
> >>> + CORE_ADDR entry_point = exec_base
> >>> + + pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint;
> >>> + info->entry_point = entry_point;
> >>> +
> >>> + breakpoint *startup_breakpoint
> >>> + = iterate_over_breakpoints ([] (breakpoint *bp)
> >>> + {
> >>> + return bp->ops == &entry_point_breakpoint_ops;
> >>> + });
> >>> + if (startup_breakpoint == nullptr)
> >>> + {
> >>> + event_location_up location
> >>> + = new_address_location (entry_point, nullptr, 0);
> >>> + create_breakpoint (target_gdbarch(), location.get(), nullptr, -1,
> >>
> >> Space before parens.
> >>
> >> This looking up for the pre-existing breakpoint doesn't work
> >> correctly when you consider multiple inferiors, where each will
> >> need a location for its own entry pointer. The Windows backend
> >> doesn't support multi-process, but OTOH, if you do it like jit.c
> >> does, which just basically always create a breakpoint and
> >> stores the pointer in the per-pspace data, you're practically
> >> good to go, and you'll make it easier for whomever comes next
> >> and decides to all multi-process support.
> >
> > I'm not sure what part here will not work.
> > I actually tested with multiple inferiors (not running at the same time),
> > and update_breakpoint_locations made a breakpoint location for each:
> >
> > (gdb) maint info br
> > Num Type Disp Enb Address What
> > -1 breakpoint del y <MULTIPLE>
> > -1.1 y 0x00000000004015b0 in mainCRTStartup at C:/gcc/src/mingw-w64-v7.0.0/mingw-w64-crt/crt/crtexe.c:218:1 inf 2
> > -1.2 y 0x000000000117ee20 in mainCRTStartup at heob.c:7094:1 inf 1
> >
>
> I looked better at the code, and as long as startup_breakpoint_re_set
> is called whenever an inferior is added, I agree it should work.
>
> However, does the breakpoint go away when the inferior exits, though?
> I'm wondering what happens when, say:
>
> #1 - you load a Windows binary in inferior 1.
> #2 - you run the inferior, and it exits
> #3 - you now load a non-Windows binary in inferior 1 (say, a GNU/Linux program)
> #4 - you run inferior 1
>
> Don't we end up in startup_breakpoint_re_set in step #3 or #4?
> If so, that calls get_windows_inferior_data() which a new
> windows_info object, and then creates a location at address 0,
> I presume. All while the inferior isn't a Windows inferior.
>
> Or what if you load a Windows program in inferior 1, and a GNU/Linux
> program in inferior 2? Doesn't a similar problem happen?
You're probably right.
I've never build a gdb which understands both Windows and Linux
executables (I think), so I can't test it.
It looks like it keeps the breakpoint on the same location as the last
Windows executable.
What would you suggest how to fix this?
Hannes
More information about the Gdb-patches
mailing list