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

Pedro Alves
2020-05-31T16:38:03Z

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.

> +
> +/* 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.

> + 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.

> + nullptr, 0, 1, bp_breakpoint, 0,
> + AUTO_BOOLEAN_FALSE, &entry_point_breakpoint_ops,
> + 0, 1, 1, 0);

Thanks,
Pedro Alves