From: Pedro Alves <palves@redhat.com>
To: Hannes Domani <ssbssa@yahoo.de>, gdb-patches@sourceware.org
Subject: Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
Date: Sun, 31 May 2020 17:37:57 +0100 [thread overview]
Message-ID: <c235efb6-06ee-6a9a-f836-15a95f164677@redhat.com> (raw)
In-Reply-To: <20200525185659.59346-8-ssbssa@yahoo.de>
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
next prev parent reply other threads:[~2020-05-31 16:38 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200525185659.59346-1-ssbssa.ref@yahoo.de>
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
2020-05-25 18:56 ` [PATCH 1/7] Fix function argument and return value locations Hannes Domani
2020-05-25 21:02 ` Simon Marchi
2020-05-25 21:32 ` Hannes Domani
2020-05-25 22:14 ` Simon Marchi
2020-05-25 23:03 ` Hannes Domani
2020-05-26 16:14 ` Simon Marchi
2020-05-26 20:43 ` Tom Tromey
2020-05-25 18:56 ` [PATCH 2/7] Handle Windows drives in auto-load script paths Hannes Domani
2020-05-26 16:04 ` Jon Turney
2020-05-26 16:31 ` Hannes Domani
2020-05-26 16:05 ` Christian Biesinger
2020-05-26 16:25 ` Hannes Domani
2020-05-26 16:31 ` Christian Biesinger
2020-05-26 16:40 ` Hannes Domani
2020-05-26 16:42 ` Christian Biesinger
2020-05-26 17:14 ` Hannes Domani
2020-05-25 18:56 ` [PATCH 3/7] Handle Windows drives in rbreak paths Hannes Domani
2020-05-25 18:56 ` [PATCH 4/7] Use errno value of first openp failure Hannes Domani
2020-05-26 20:37 ` Tom Tromey
2020-05-25 18:56 ` [PATCH 5/7] Close file handle of empty history file Hannes Domani
2020-05-26 16:37 ` Christian Biesinger
2020-05-26 17:42 ` Hannes Domani
2020-05-27 14:33 ` Tom Tromey
2020-05-27 17:37 ` Hannes Domani
2020-05-27 18:27 ` Christian Biesinger
2020-05-25 18:56 ` [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR Hannes Domani
2020-05-26 20:45 ` Tom Tromey
2020-05-27 17:50 ` Hannes Domani
2020-05-25 18:56 ` [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point Hannes Domani
2020-05-27 12:07 ` Pedro Alves
2020-05-27 14:48 ` Pedro Alves
2020-05-27 15:39 ` Hannes Domani
2020-05-31 15:54 ` Pedro Alves
2020-05-31 16:37 ` Pedro Alves [this message]
2020-06-07 12:56 ` Hannes Domani
2020-07-08 17:43 ` Hannes Domani
2020-10-09 18:22 ` Pedro Alves
2020-10-09 18:51 ` Hannes Domani
2020-10-12 11:13 ` Pedro Alves
2020-10-12 17:21 ` Tom Tromey
2020-10-12 17:22 ` Tom Tromey
2020-05-28 18:15 ` Windows testsuite failures Christian Biesinger
2020-05-28 18:37 ` Hannes Domani
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c235efb6-06ee-6a9a-f836-15a95f164677@redhat.com \
--to=palves@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=ssbssa@yahoo.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).