[RFC] [gdb/breakpoints] Use gdbarch_addr_bits_remove in breakpoint_address_match
Luis Machado
luis.machado@arm.com
Fri Aug 30 07:21:06 GMT 2024
On 8/29/24 11:20, Tom de Vries wrote:
> Consider a hello world exec on arm-linux (specifically debian, which is
> relevant because it strips minimal symbols from shared libraries), compiled
> with -marm:
I suppose excessive stripping would be to blame here, right? The thumb-checking logic
is really dependent on the symbol information so we can have mapping symbols. If
those are not available, then it is hard to guess the thumbness correctly.
> ...
> $ gcc -g -marm hello.c
> ...
> and started in gdb:
> ...
> $ gdb -q a.out -ex start
> Reading symbols from a.out...
> Temporary breakpoint 1 at 0x50c: file hello.c, line 6.
> Starting program: a.out
>
> Temporary breakpoint 1, main () at hello.c:6
> 6 printf ("hello\n");
> (gdb)
> ...
>
> The link register contains the address main returns to:
> ...
> (gdb) p /x $lr
> $1 = 0xf7ec43db
This is actually a thumb address with the LSB set (to indicate it is a thumb address).
The procedure is to remove that bit internally and use the thumb address instead.
> ...
> which is some thumb address in libc.
>
> We can step to it, but say we want to set a breakpoint on it, and continue.
>
> First, let's try with the non-thumb address:
> ...
> (gdb) b *0xf7ec43da
> Breakpoint 2 at 0xf7ec43da
> (gdb) c
> Continuing.
> hello
>
> Program received signal SIGSEGV, Segmentation fault.
> 0xf7ec43c8 in ?? () from /lib/arm-linux-gnueabihf/libc.so.6
> (gdb)
> ...
>
> So what happened? In arm_breakpoint_kind_from_pc, arm_pc_is_thumb is called
> which as last resort falls back to "arm fallback-mode auto", which (given that
> we're in the frame of main, which is an arm frame) returns false.
I find the above a bit strange. We clearly have a thumb address, so the fact gdb is
going with a regular breakpoint is odd. This case should've worked without having
to do anything extra.
>
> That is the wrong answer, so we're choosing the wrong kind of breakpoint and
> things go downhill from there.
>
> We can workaround this by doing "set arm fallback-mode thumb", but say we
> don't want that because that's a bad default for our current situation.
>
> So, let's now try with the thumb address:
> ...
> (gdb) b *0xf7ec43db
> Breakpoint 2 at 0xf7ec43db
> (gdb) c
> Continuing.
> hello
> <hangs>
> ...
>
The above works, but I don't think it is correct. All we're doing is providing a hint that
a particular address is thumb by setting the LSB to 1, so gdb know what to do, but
address 0xf7ec43db isn't really a thing.
> In this case, we get the correct breakpoint kind, but the breakpoint doesn't
> trigger because the address doesn't match.
>
> More specifically, in handle_signal_stop we do:
> ...
> ecs->event_thread->set_stop_pc
> (regcache_read_pc (get_thread_regcache (ecs->event_thread)));
> ...
> and regcache_read_pc calls gdbarch_addr_bits_remove so
> ecs->event_thread->stop_pc () == 0xf7ec43da, and that's the address
> that gets compared to breakpoint address 0xf7ec43db in
> breakpoint_address_match.
>
> Fix this by using gdbarch_addr_bits_remove in breakpoint_address_match, such
> that we get:
> ...
> (gdb) c
> Continuing.
> hello
>
> Breakpoint 2, 0xf7ec43da in ?? () from /lib/arm-linux-gnueabihf/libc.so.6
> (gdb)
> ...
>
> Likewise in breakpoint_address_match_range.
>
> Note: if we would hoist the call to gdbarch_addr_bits_remove in
> arm_adjust_breakpoint_address like this:
> ...
> map_type = arm_find_mapping_symbol (bpaddr, &boundary);
> +
> + bpaddr = gdbarch_addr_bits_remove (gdbarch, bpaddr);
> +
> if (map_type == 0)
> /* Thumb-2 code must have mapping symbols to have a chance. */
> return bpaddr;
>
> - bpaddr = gdbarch_addr_bits_remove (gdbarch, bpaddr);
> ...
> we would not need to change breakpoint_address_match, but doing so would
> merely transform the problem into the non-thumb address case shown above, and
> we'd run into a SIGSEGV or SIGILL or some such.
>
> If however we make the arm target:
> - remember the addr bits it removed, and
> - recall those in arm_pc_is_thumb
> then his approach would work and there'd be no need for changes outside the
> target.
>
> However, note that say you make a mistake and first do break *0xf00f001, while
> the address is not thumb and you should have used *0xf00f000, then a subsequent
> "break *0xf00f000" also gets a thumb breakpoint. So you'd need a means to
> correct these kind of mistakes and make this caching explicit to the user,
> which is not great.
>
> Tested on arm-linux.
> ---
> gdb/breakpoint.c | 26 +++++--
> .../gdb.arch/thumb-address-breakpoint.c | 36 ++++++++++
> .../gdb.arch/thumb-address-breakpoint.exp | 71 +++++++++++++++++++
> 3 files changed, 127 insertions(+), 6 deletions(-)
> create mode 100644 gdb/testsuite/gdb.arch/thumb-address-breakpoint.c
> create mode 100644 gdb/testsuite/gdb.arch/thumb-address-breakpoint.exp
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 17bd627f867..1558627c9a3 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7288,9 +7288,16 @@ int
> breakpoint_address_match (const address_space *aspace1, CORE_ADDR addr1,
> const address_space *aspace2, CORE_ADDR addr2)
> {
> - return ((gdbarch_has_global_breakpoints (current_inferior ()->arch ())
> - || aspace1 == aspace2)
> - && addr1 == addr2);
> + struct gdbarch *gdbarch = current_inferior ()->arch ();
> +
> + bool same_aspace = (aspace1 == aspace2
> + || gdbarch_has_global_breakpoints (gdbarch));
> + if (!same_aspace)
> + return false;
> +
> + addr1 = gdbarch_addr_bits_remove (gdbarch, addr1);
> + addr2 = gdbarch_addr_bits_remove (gdbarch, addr2);
> + return addr1 == addr2;
> }
>
> /* Returns true if {ASPACE2,ADDR2} falls within the range determined by
> @@ -7304,9 +7311,16 @@ breakpoint_address_match_range (const address_space *aspace1,
> int len1, const address_space *aspace2,
> CORE_ADDR addr2)
> {
> - return ((gdbarch_has_global_breakpoints (current_inferior ()->arch ())
> - || aspace1 == aspace2)
> - && addr2 >= addr1 && addr2 < addr1 + len1);
> + struct gdbarch *gdbarch = current_inferior ()->arch ();
> +
> + bool same_aspace = (aspace1 == aspace2
> + || gdbarch_has_global_breakpoints (gdbarch));
> + if (!same_aspace)
> + return false;
> +
> + addr1 = gdbarch_addr_bits_remove (gdbarch, addr1);
> + addr2 = gdbarch_addr_bits_remove (gdbarch, addr2);
> + return addr2 >= addr1 && addr2 < addr1 + len1;
> }
>
> /* Returns true if {ASPACE,ADDR} matches the breakpoint BL. BL may be
> diff --git a/gdb/testsuite/gdb.arch/thumb-address-breakpoint.c b/gdb/testsuite/gdb.arch/thumb-address-breakpoint.c
> new file mode 100644
> index 00000000000..30e09b295dc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/thumb-address-breakpoint.c
> @@ -0,0 +1,36 @@
> +/* Copyright 2024 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +__attribute__((target("arm")))
> +static void
> +bar (void)
> +{
> +}
> +
> +__attribute__((target("thumb")))
> +void
> +foo (void)
> +{
> + bar ();
> +}
> +
> +int
> +main (void)
> +{
> + foo ();
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/thumb-address-breakpoint.exp b/gdb/testsuite/gdb.arch/thumb-address-breakpoint.exp
> new file mode 100644
> index 00000000000..a024f3728b1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/thumb-address-breakpoint.exp
> @@ -0,0 +1,71 @@
> +# Copyright 2024 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +require is_aarch32_target
> +
> +standard_testfile
> +
> +if { [build_executable "failed to prepare" $testfile $srcfile {nodebug}] } {
> + return -1
> +}
> +
> +# Generate $binfile.stripped.
> +set objcopy_program [gdb_find_objcopy]
> +set objcopy_args "--strip-all --keep-symbol=bar"
> +set cmd "$objcopy_program $objcopy_args $binfile $binfile.stripped"
> +set result [catch "exec $cmd" output]
> +if { $result != 0 } {
> + verbose -log "result is $result"
> + verbose -log "output is $output"
> + return -1
> +}
> +
> +clean_restart $binfile.stripped
> +
> +# We're able to set a breakpoint on bar because we didn't strip that symbol.
> +# Bar is an arm function. This prevents "arm fallback-mode auto" from
> +# magically declaring addresses to be thumb, at least while we're in that
> +# frame.
> +if { ![runto bar] } {
> + return -1
> +}
> +
> +# Get the link register, pointing to foo, a thumb function.
> +set lr ""
> +gdb_test_multiple {p /x $lr} "" {
> + -re -wrap "= ($hex)" {
> + set lr $expect_out(1,string)
> + pass $gdb_test_name
> + }
> +}
> +
> +if { $lr == "" } {
> + return -1
> +}
> +
> +# Assert that $lr is a thumb address.
> +gdb_assert { [expr $lr & 1 == 1] }
> +
> +# Set a breakpoint on $lr.
> +gdb_breakpoint "*$lr"
> +
> +# With unstripped $binfile, the thumb bit would have been dropped by
> +# arm_adjust_breakpoint_address. Check that the breakpoint address still
> +# contains the thumb bit. This was a necessary condition to trigger the hang.
> +set re_lr [regsub 0x0* $lr ""]
> +gdb_test "info break" $re_lr.*
> +
> +# Now check that the breakpoint actually works. This used to hang.
> +gdb_continue_to_breakpoint "foo"
>
> base-commit: 8ee4561d38f2b35cc5068ad3b39f839c2847b5d4
More information about the Gdb-patches
mailing list