[RFC,gdb/breakpoints] Use gdbarch_addr_bits_remove in breakpoint_address_match

  Consider a hello world exec on arm-linux (specifically debian, which is
relevant because it strips minimal symbols from shared libraries), compiled
with -marm:
...
$ 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
...
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.

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

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


base-commit: 8ee4561d38f2b35cc5068ad3b39f839c2847b5d4
  

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
  

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
>
  

On 8/30/24 09:25, Tom de Vries wrote:
> 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.
> ...
> 

That's true. Still, the use of that switch is a bit cumbersome as it is either one way or the
other. It means users need to know the instructions to make an informed decision. Otherwise
you will always run into issues when you force one mode but end up having another (in mixed
thumb/arm code).

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

To expand on it, the LSB being set is another way we communicate the thumbness of a PC
address. In this case we're telling gdb the return address is thumb, because there isn't
symbol information for these return addresses sometimes.

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

True. That could be improved I suppose. The current system is crumbling a bit because
distros want to have compact packages and will often strip things.


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

Right.

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

Yes, very likely.

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

Sorry, I glanced at it and thought gdb was stopping (assuming it had removed the LSB and placed a thumb
address, which is not the case).

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

It is difficult to have a proper solution to determining thumbness on 32-bit arm. Given
the improvement, I don't see a problem with this patch, so unless others have objections...

Approved-By: Luis Machado <luis.machado@arm.com>

Thanks!
  

On 8/30/24 10:53, Luis Machado wrote:
> It is difficult to have a proper solution to determining thumbness on 32-bit arm. Given
> the improvement, I don't see a problem with this patch, so unless others have objections...
> 
> Approved-By: Luis Machado<luis.machado@arm.com>

Hi Luis,

thanks for the review.

Just to re-iterate in more detail what I mentioned in a recent off-list 
conversation.

I haven't committed this patch, because while analyzing another failure 
on arm-linux I came to realize that there are other places in gdb that 
compare (breakpoint) addresses that do not use breakpoint_address_match.

Fixing all of those spots would be some work, but I'm not sure it's a 
good idea to begin with, because it would also be difficult to maintain 
if this only gets us in trouble on one target, in absence of minimal 
symbols, which happens on a limited amount of distros.

So until I have re-convinced myself that this is the way to go, I'm 
postponing committing this.

Thanks,
- Tom
  

On 9/23/24 09:30, Tom de Vries wrote:
> On 8/30/24 10:53, Luis Machado wrote:
>> It is difficult to have a proper solution to determining thumbness on 32-bit arm. Given
>> the improvement, I don't see a problem with this patch, so unless others have objections...
>>
>> Approved-By: Luis Machado<luis.machado@arm.com>
> 
> Hi Luis,
> 
> thanks for the review.
> 
> Just to re-iterate in more detail what I mentioned in a recent off-list conversation.
> 
> I haven't committed this patch, because while analyzing another failure on arm-linux I came to realize that there are other places in gdb that compare (breakpoint) addresses that do not use breakpoint_address_match.
> 
> Fixing all of those spots would be some work, but I'm not sure it's a good idea to begin with, because it would also be difficult to maintain if this only gets us in trouble on one target, in absence of minimal symbols, which happens on a limited amount of distros.
> 
> So until I have re-convinced myself that this is the way to go, I'm postponing committing this.
> 
> Thanks,
> - Tom

Thanks for the heads-up. There might be ways to improve the situation somewhat, but unfortunately reliability depends on the presence of the thumb mapping symbols.