Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Hannes Domani via Gdb-patches <gdb-patches@sourceware.org>]

 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