Re: [PATCHv2 3/6] gdb/arm: avoid undefined behaviour in arm_frame_is_thumb

[Andrew Burgess <aburgess@redhat.com>]

Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 6/10/22 14:08, Andrew Burgess via Gdb-patches wrote:
>> This commit fixes real undefined behaviour in GDB which I spotted when
>> working on a later patch in this series.  The later patch in this
>> series detects when the result of gdbarch_tdep() is cast to the wrong
>> type.
>> 
>> The issue is revealed by the gdb.multi/multi-arch.exp test.
>> 
>> In this test we setup two inferiors, an AArch64 process, and an ARM
>> process, then at one point we have inferior 1 selected (the AArch64
>> inferior), and we place a breakpoint on a symbol present in the other
>> inferior (the ARM inferior).
>> 
>> During the process of creating the breakpoint we call arm_pc_is_thumb,
>> the GDBARCH passed into this function is correct, that is, represents
>> the ARM process.
>> 
>> For whatever reason we are unable to figure out if the address in
>> question is thumb or not throughout most of arm_pc_is_thumb, and so we
>> get to this code at the end of the function:
>> 
>>    /* If we couldn't find any symbol, but we're talking to a running
>>       target, then trust the current value of $cpsr.  This lets
>>       "display/i $pc" always show the correct mode (though if there is
>>       a symbol table we will not reach here, so it still may not be
>>       displayed in the mode it will be executed).  */
>>    if (target_has_registers ())
>>      return arm_frame_is_thumb (get_current_frame ());
>> 
>> Which I guess is a last attempt to figure out the thumb status of an
>> address.  However, remember, we the AArch64 inferior is current at
>> this time, so the current frame is an AArch64 frame.
>
> If we're trying to insert a breakpoint into a 32-bit inferior,
> we should really have the 32-bit arm gdbarch at hand, not the AArch64
> gdbarch.

We do.

>
> I think the bug is somewhere else, in whoever passed the current inferior's gdbarch
> as opposed to the gdbarch of the inferior that contains the symbol
> we've found.

We do pass in the gdbarch of the inferior containing the breakpoint location.

If I change the condition to an assert, then run
gdb.multi/multi-arch.exp and catch the assertion, the stack looks like
this:

  #0  internal_error (file=0x55834840c8 "../../src/gdb/arm-tdep.c", line=551, 
      fmt=0x5583483d70 "%s: Assertion `%s' failed.") at ../../src/gdbsupport/errors.cc:51
  #1  0x0000005582b0e6a4 in arm_frame_is_thumb (frame=0x55c0849a00) at ../../src/gdb/arm-tdep.c:551
  #2  0x0000005582b0eb70 in arm_pc_is_thumb (gdbarch=0x55c0937080, memaddr=4195692) at ../../src/gdb/arm-tdep.c:687
  #3  0x0000005582b17e70 in arm_adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692)
      at ../../src/gdb/arm-tdep.c:4925
  #4  0x0000005582af0a60 in gdbarch_adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692)
      at ../../src/gdb/gdbarch.c:2840
  #5  0x0000005582b73c90 in adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692, bptype=bp_breakpoint)
      at ../../src/gdb/breakpoint.c:7147
  #6  0x0000005582b76854 in code_breakpoint::add_location (this=0x55c088e340, sal=...)
      at ../../src/gdb/breakpoint.c:8100
  #7  0x0000005582b773b8 in code_breakpoint::code_breakpoint (this=0x55c088e340, gdbarch_=0x55c089c050, 
      type_=bp_breakpoint, sals=..., location_=..., filter_=std::unique_ptr<char> = {...}, 
      cond_string_=std::unique_ptr<char> = {...}, extra_string_=std::unique_ptr<char> = {...}, 
      disposition_=disp_donttouch, thread_=-1, task_=0, ignore_count_=0, from_tty=1, enabled_=1, flags=0, 
      display_canonical_=0) at ../../src/gdb/breakpoint.c:8329
  #8  0x0000005582b90c00 in ordinary_breakpoint::code_breakpoint (this=0x55c088e340) at ../../src/gdb/breakpoint.c:266
  #9  0x0000005582b88758 in new_breakpoint_from_type<gdb::array_view<symtab_and_line const>&, std::unique_ptr<event_location, event_location_deleter>, std::unique_ptr<char, gdb::xfree_deleter<char> >, std::unique_ptr<char, gdb::xfree_deleter<char> >, std::unique_ptr<char, gdb::xfree_deleter<char> >, bpdisp&, int&, int&, int&, int&, int&, unsigned int&, int&> (gdbarch=0x55c089c050, type=bp_breakpoint) at ../../src/gdb/breakpoint.c:1303
  #10 0x0000005582b77710 in create_breakpoint_sal (gdbarch=0x55c089c050, sals=..., location=..., 
      filter=std::unique_ptr<char> = {...}, cond_string=std::unique_ptr<char> = {...}, 
      extra_string=std::unique_ptr<char> = {...}, type=bp_breakpoint, disposition=disp_donttouch, thread=-1, task=0, 
      ignore_count=0, from_tty=1, enabled=1, internal=0, flags=0, display_canonical=0)
      at ../../src/gdb/breakpoint.c:8395
  #11 0x0000005582b779d8 in create_breakpoints_sal (gdbarch=0x55c089c050, canonical=0x7fd8d196c0, 
      cond_string=std::unique_ptr<char> = {...}, extra_string=std::unique_ptr<char> = {...}, type=bp_breakpoint, 
      disposition=disp_donttouch, thread=-1, task=0, ignore_count=0, from_tty=1, enabled=1, internal=0, flags=0)
      at ../../src/gdb/breakpoint.c:8438
  #12 0x0000005582b78d94 in create_breakpoint (gdbarch=0x55c089c050, location=0x55c0979750, cond_string=0x0, thread=-1, 
      extra_string=0x0, force_condition=false, parse_extra=1, tempflag=0, type_wanted=bp_breakpoint, ignore_count=0, 
      pending_break_support=AUTO_BOOLEAN_AUTO, ops=0x558389c650 <code_breakpoint_ops>, from_tty=1, enabled=1, 
      internal=0, flags=0) at ../../src/gdb/breakpoint.c:8923
  #13 0x0000005582b792d8 in break_command_1 (arg=0x55c0732fc2 "", flag=0, from_tty=1) at ../../src/gdb/breakpoint.c:8994
  #14 0x0000005582b79578 in break_command (arg=0x55c0732fb6 "hangout_loop", from_tty=1)
      at ../../src/gdb/breakpoint.c:9065

In frame #6 we select a gdbarch based on the location of the breakpoint,
its at this point that we select the bfd_arch_arm gdbarch.

For frames #5, #4, #3, and #2 we are passing in a bfd_arch_arm gdbarch,
which is correct, and what you are asking for.

The problem is that in frame #2 we fail to find any of the special hints
that indicate if the code is thumb or not.  Now, _maybe_ you could argue
that one of the conditions in arm_pc_is_thumb should trigger, but, for
me, the very fact that there is a "catch all" case at the end of the
function means we have to be open to the possibility that non of the
special symbols, or bottom bit of the address set cases might trigger.

And so, we get to this code in arm_pc_is_thumb:

  if (target_has_registers ())
    return arm_frame_is_thumb (get_current_frame ());

At this point GDBARCH _is_ a bfd_arch_arm architecture.

But, the current frame is bfd_arch_aarch64.

And so, we enter frame #1, arm_frame_is_thumb, passing in a frame that
is not bfd_arch_arm.

So, are you are suggesting we should switch frames as part of the
breakpoint setting process?  Because I'm not sure how you'd pick even a
suitable inferior, multiple ARM inferiors might share a single program
space, so we could have a single gdbarch, but multiple inferiors, each
with multiple threads, and each thread with multiple frames...

I guess we could push the architecture check out of arm_frame_is_thumb
back to arm_pc_is_thumb, and only call arm_frame_is_thumb for the case
where the architecture is ARM... that doesn't make sense to me, but
maybe, I guess...

Anyway, let me know if the above makes any more sense.  If it does I can
update the commit message.

Thanks,
Andrew