[PATCH] [gdb/tdep] Use cached breakpoint kind in breakpoint_kind

[PATCH] [gdb/tdep] Use cached breakpoint kind in breakpoint_kind

[Tom de Vries]

With test-case gdb.base/solib-probes-nosharedlibrary.exp on arm-linux, I get:
...
(gdb) continue^M
Continuing.^M
^M
Program received signal SIGILL, Illegal instruction.^M
0xf7fd267a in ?? ()^M
(gdb) FAIL: $exp: continue to breakpoint: main
...

This happens as follows:
- the starti command is called,
- gdb loads symbols for ld.so,
- the "shlib events" breakpoint is inserted at the address of _dl_debug_state,
- execution stops at ld.so _start, and the inserted breakpoint is removed,
- the nosharedlibrary command is called,
- gdb forgets about all shared library symbols,
- the continue command is called,
- gdb re-inserts the "shlib events" breakpoint, but the call to
  breakpoint_kind returns the wrong value: ARM_BP_KIND_ARM instead of
  ARM_BP_KIND_THUMB, so the wrong type of breakpoint is inserted, and
- when execution hits the breakpoint, the SIGILL triggers.

In order to correctly set a breakpoint on an address for arm, gdb needs to
known whether that address is in thumb or arm code, and the problem is that
that information is no longer available due to the nosharedlibrary command.

However, the breakpoint kind is is available from the previous time the
breakpoint was inserted.

Fix this in breakpoint_kind, by using the info cached in bl->target_info.kind,
if available.

Tested on arm-linux.

PR tdep/31817
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31817
---
 gdb/breakpoint.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a973518ac5f..0c99ccc9568 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2792,7 +2792,11 @@ breakpoint_kind (const struct bp_location *bl, CORE_ADDR *addr)
 							 regcache, addr);
     }
   else
-    return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
+    {
+      if (bl->target_info.kind != 0)
+	return bl->target_info.kind;
+      return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
+    }
 }
 
 /* Rethrow the currently handled exception, if it's a TARGET_CLOSE_ERROR.

base-commit: 3a659c2a30f40c2a734fda5566098804b5ee38fc
-- 
2.35.3

[PING][PATCH] [gdb/tdep] Use cached breakpoint kind in breakpoint_kind

[Tom de Vries]

On 6/7/24 16:38, Tom de Vries wrote:
> With test-case gdb.base/solib-probes-nosharedlibrary.exp on arm-linux, I get:
> ...
> (gdb) continue^M
> Continuing.^M
> ^M
> Program received signal SIGILL, Illegal instruction.^M
> 0xf7fd267a in ?? ()^M
> (gdb) FAIL: $exp: continue to breakpoint: main
> ...
> 
> This happens as follows:
> - the starti command is called,
> - gdb loads symbols for ld.so,
> - the "shlib events" breakpoint is inserted at the address of _dl_debug_state,
> - execution stops at ld.so _start, and the inserted breakpoint is removed,
> - the nosharedlibrary command is called,
> - gdb forgets about all shared library symbols,
> - the continue command is called,
> - gdb re-inserts the "shlib events" breakpoint, but the call to
>    breakpoint_kind returns the wrong value: ARM_BP_KIND_ARM instead of
>    ARM_BP_KIND_THUMB, so the wrong type of breakpoint is inserted, and
> - when execution hits the breakpoint, the SIGILL triggers.
> 
> In order to correctly set a breakpoint on an address for arm, gdb needs to
> known whether that address is in thumb or arm code, and the problem is that
> that information is no longer available due to the nosharedlibrary command.
> 
> However, the breakpoint kind is is available from the previous time the
> breakpoint was inserted.
> 
> Fix this in breakpoint_kind, by using the info cached in bl->target_info.kind,
> if available.
> 

Ping.

Thanks,
- Tom

> Tested on arm-linux.
> 
> PR tdep/31817
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31817
> ---
>   gdb/breakpoint.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index a973518ac5f..0c99ccc9568 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2792,7 +2792,11 @@ breakpoint_kind (const struct bp_location *bl, CORE_ADDR *addr)
>   							 regcache, addr);
>       }
>     else
> -    return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
> +    {
> +      if (bl->target_info.kind != 0)
> +	return bl->target_info.kind;
> +      return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
> +    }
>   }
>   
>   /* Rethrow the currently handled exception, if it's a TARGET_CLOSE_ERROR.
> 
> base-commit: 3a659c2a30f40c2a734fda5566098804b5ee38fc

Re: [PING][PATCH] [gdb/tdep] Use cached breakpoint kind in breakpoint_kind

[Tom de Vries]

On 7/31/24 13:26, Tom de Vries wrote:
> On 6/7/24 16:38, Tom de Vries wrote:
>> With test-case gdb.base/solib-probes-nosharedlibrary.exp on arm-linux, 
>> I get:
>> ...
>> (gdb) continue^M
>> Continuing.^M
>> ^M
>> Program received signal SIGILL, Illegal instruction.^M
>> 0xf7fd267a in ?? ()^M
>> (gdb) FAIL: $exp: continue to breakpoint: main
>> ...
>>
>> This happens as follows:
>> - the starti command is called,
>> - gdb loads symbols for ld.so,
>> - the "shlib events" breakpoint is inserted at the address of 
>> _dl_debug_state,
>> - execution stops at ld.so _start, and the inserted breakpoint is 
>> removed,
>> - the nosharedlibrary command is called,
>> - gdb forgets about all shared library symbols,
>> - the continue command is called,
>> - gdb re-inserts the "shlib events" breakpoint, but the call to
>>    breakpoint_kind returns the wrong value: ARM_BP_KIND_ARM instead of
>>    ARM_BP_KIND_THUMB, so the wrong type of breakpoint is inserted, and
>> - when execution hits the breakpoint, the SIGILL triggers.
>>
>> In order to correctly set a breakpoint on an address for arm, gdb 
>> needs to
>> known whether that address is in thumb or arm code, and the problem is 
>> that
>> that information is no longer available due to the nosharedlibrary 
>> command.
>>
>> However, the breakpoint kind is is available from the previous time the
>> breakpoint was inserted.
>>
>> Fix this in breakpoint_kind, by using the info cached in 
>> bl->target_info.kind,
>> if available.
>>
> 
> Ping.
> 

I've given this patch some more thought, and I think I have come up with 
a counter example.

Say you have a breakpoint kind that is different depending on the 
alignment, one kind for "addr & 0x4 == 0x0" and another kind for "addr & 
0x4 == 0x2".

Then when relocating the exec, the kind might change, but due to the 
caching introduced by this patch, we might be incorrectly using the same 
kind as before the relocation.

So, I don't think that this patch is safe.

I came up with an alternative solution that doesn't cache the kind, but 
lets the tdep part cache data alongside an address if that is safe and 
beneficial.

Submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2024-August/210884.html ).

Thanks,
- Tom

> Thanks,
> - Tom
> 
>> Tested on arm-linux.
>>
>> PR tdep/31817
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31817
>> ---
>>   gdb/breakpoint.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index a973518ac5f..0c99ccc9568 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -2792,7 +2792,11 @@ breakpoint_kind (const struct bp_location *bl, 
>> CORE_ADDR *addr)
>>                                regcache, addr);
>>       }
>>     else
>> -    return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
>> +    {
>> +      if (bl->target_info.kind != 0)
>> +    return bl->target_info.kind;
>> +      return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
>> +    }
>>   }
>>   /* Rethrow the currently handled exception, if it's a 
>> TARGET_CLOSE_ERROR.
>>
>> base-commit: 3a659c2a30f40c2a734fda5566098804b5ee38fc
> 

Re: [PING][PATCH] [gdb/tdep] Use cached breakpoint kind in breakpoint_kind

[Simon Marchi]

On 8/5/24 12:00 PM, Tom de Vries wrote:
> I've given this patch some more thought, and I think I have come up
> with a counter example.
> 
> Say you have a breakpoint kind that is different depending on the
> alignment, one kind for "addr & 0x4 == 0x0" and another kind for "addr
> & 0x4 == 0x2".
> 
> Then when relocating the exec, the kind might change, but due to the
> caching introduced by this patch, we might be incorrectly using the
> same kind as before the relocation.

Did you see this kind of relocation in practice?  Typically, executables
get relocated with an offset that is a multiple of the page size, or
something big like that, so it wouldn't affect alignment in such way.

Simon

Re: [PING][PATCH] [gdb/tdep] Use cached breakpoint kind in breakpoint_kind

[Tom de Vries]

On 8/5/24 19:08, Simon Marchi wrote:
> On 8/5/24 12:00 PM, Tom de Vries wrote:
>> I've given this patch some more thought, and I think I have come up
>> with a counter example.
>>
>> Say you have a breakpoint kind that is different depending on the
>> alignment, one kind for "addr & 0x4 == 0x0" and another kind for "addr
>> & 0x4 == 0x2".
>>
>> Then when relocating the exec, the kind might change, but due to the
>> caching introduced by this patch, we might be incorrectly using the
>> same kind as before the relocation.
> 
> Did you see this kind of relocation in practice?  Typically, executables
> get relocated with an offset that is a multiple of the page size, or
> something big like that, so it wouldn't affect alignment in such way.

It's a theoretical example, so it's easy to adapt it be affected by 
relocation with page size multiples, but it remains theoretical.

Thanks,
- Tom