* [PING v2][PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
[not found] <990229213.2562185.1601818489842.ref@mail.yahoo.com>
@ 2020-10-04 13:34 ` Hannes Domani
0 siblings, 0 replies; only message in thread
From: Hannes Domani @ 2020-10-04 13:34 UTC (permalink / raw)
To: Gdb-patches; +Cc: Pedro Alves
Am Mittwoch, 8. Juli 2020, 19:43:52 MESZ hat Hannes Domani <ssbssa@yahoo.de> Folgendes geschrieben:
Ping.
> Ping.
>
> Am Sonntag, 7. Juni 2020, 14:56:29 MESZ hat Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:
>
> > 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?
> >
> >
> > > > +
> > > > +/* 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
^ permalink raw reply [flat|nested] only message in thread