[RFC] [gdb/breakpoints] Use gdbarch_addr_bits_remove in breakpoint_address_match

[Tom de Vries]

On 8/30/24 09:21, Luis Machado wrote:
> 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.
> 

It is hard to guess indeed, but that doesn't mean that gdb doesn't try 
to take care of that situation.

For instance, we have arm fallback-mode which:
...
controls GDB’s default behavior when the symbol table is not available.
...

>> ...
>> $ 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).
> 

I see, I got that mixed up, thanks for pointing that out.

> The procedure is to remove that bit internally and use the thumb address instead.
> 

With which you currently throw away the information that it's a thumb 
address.

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

Um, you mean because it's not 4-byte aligned:
...
(gdb) p /x 0xf7ec43da & 0x3 == 0
$5 = 0x0
...
?

That's not taken into account in arm_pc_is_thumb.

So, we could add that, but that would only work in half of the cases 
(which is perhaps why it hasn't been added).

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

The breakpoint doesn't trigger, so I don't think that it works.  The 
proposed patch does make it work.

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

I don't see it any different as assuming "arm fallback-mode thumb" for 
one specific address.

Thanks,
- Tom

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