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

[Tom de Vries <tdevries@suse.de>]

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
>