Re: Restoring pc to a different value than lr on aarch64

[Yichao Yu <yyc1992@gmail.com>]

On Wed, May 11, 2022 at 10:52 AM Luis Machado <luis.machado@arm.com> wrote:
>
> On 5/11/22 14:26, Yichao Yu wrote:
> > On Tue, May 10, 2022 at 10:49 AM Luis Machado <luis.machado@arm.com> wrote:
> >>
> >> On 5/9/22 15:24, Yichao Yu wrote:
> >>> On Mon, May 9, 2022 at 6:44 AM Luis Machado <luis.machado@arm.com> wrote:
> >>>>
> >>>> On 5/6/22 17:30, Yichao Yu wrote:
> >>>>> On Fri, May 6, 2022 at 12:11 PM Yichao Yu <yyc1992@gmail.com> wrote:
> >>>>>>
> >>>>>>>> Actually I misspoke for that. It seems that sp is probably fine and
> >>>>>>>> the only thing missing causing pc to not work is that
> >>>>>>>> aarch64_dwarf_reg_to_regnum doesn't understand the PC dwarf reg
> >>>>>>>> number. It seems that the only thing needed is to add a
> >>>>>>>>
> >>>>>>>> +  if (reg == AARCH64_DWARF_PC)
> >>>>>>>> +    return AARCH64_PC_REGNUM;
> >>>>>>>>
> >>>>>>>> to that function.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Yes, GDB always assumes the PC from the previous frame is the LR from
> >>>>>>> the current frame. That is what GCC generates.
> >>>>>>>
> >>>>>>> If a different setup is needed, GDB needs to be taught about it.
> >>>>>>
> >>>>>> I agree the current code makes sense for what gcc generates. However,
> >>>>>> I think given the document from arm, explicitly setting the PC value
> >>>>>> in the unwind info should also work.
> >>>>>> Would a patch similar to the one above be acceptable to fix this issue?
> >>>>>>
> >>>>>> A related issue is that gdb also seems to be ignoring the return
> >>>>>> address register in CIE. There is at least one use of it in glibc[2]
> >>>>>> where the return address register is set to x15 instead.
> >>>>>> I've verified that gdb is currently unable to unwind after the call to
> >>>>>> `strlen` from `rawmemchr` even though the return address is still in
> >>>>>> x15.
> >>>>>> I thought this can be fixed by chaiming that PC is RA just like the
> >>>>>> fallback case but that is apparently not working...
> >>>>>
> >>>>> Actually this did work but the address is wrong before the value was
> >>>>> written to x15... So it was just due to incorrect unwind info (the
> >>>>> glibc implementation should specify how to find x15 on the entry of
> >>>>> rawmemchr).
> >>>>> Is the current implementation due to some edge cases? (like old
> >>>>> compiler version doesn't put a valid value in the CIE for the return
> >>>>> address register). It seems that many other architecture simply use
> >>>>> _RA so I don't see why this would have broader problems...
> >>
> >> This looks like a genuine bug, one that is not hit that often given it
> >> is not very common for frame to change the return address column. This
> >> will need to be fixed eventually. Thanks for spotting that.
> >>
> >>>>
> >>>> It is probably historical that this has been handled like this. If there
> >>>> is a use case for having a PC column containing distinct data, then we
> >>>> could support that.
> >>>
> >>> I couldn't find any existed code using this, but it seems that this is
> >>> the intent from ARM and I really can't think of any other way to
> >>> restore everything including both pc and lr so I'd like to support
> >>> that at least....
> >>>
> >>>> It wouldn't be as simple as that change you mentioned though, as other
> >>>> parts of the code assume PC comes from the LR.
> >>
> >> I take that back. I investigated this further and I think this should
> >> work, although it is a use case that is not so common.
> >>
> >>>
> >>> There are indeed other unwinders (the prologue unwinder) that assumes
> >>> this, which I think should be fine. Specifying the PC explicitly
> >>> should only apply to the dwarf unwinder. The only other place where I
> >>> can find that relies on this is aarch64_gen_return_address. It doesn't
> >>> seem that this function is used in most of the other logics and if the
> >>> return address column is somehow accessible from here it should also
> >>> not be too hard to fix.
> >>
> >> I think it is only the case for the dwarf unwinder, as that is the
> >> unwinder that has access to the CFI information.
> >
> > Sorry what did you mean by "it" here? (being able to handle PC != LR
> > during unwinding?) Do I need to worry about gen_return_address?
> >
>
> Sorry, I meant that we only need to worry about the dwarf unwinder
> functions. The other code not relying on CFI information doesn't need to
> be touched (prologue unwinder, aarch64_gen_return_address etc).
>
> At this point, we'd like to augment aarch64_dwarf_reg_to_regnum to
> return a valid number for the dwarf PC register.

... and I just sent out the patch
https://sourceware.org/pipermail/gdb-patches/2022-May/189035.htmlHopefully
I did it more or less right (both in the email and in the patch
itself)
BTW, is there a way to see what gdb actually outputs during the test?
Setting `verbose` doesn't seem to help. I couldn't figure out why `p
$pc` prints `<main+8>` when I used it directly but doesn't seem to in
the test... `$pc - &main` works which is probably better and is what
I'm currently using.

> >>>
> >>> Is there any other cases that I'm missing? I've also tested with my
> >>> simple change and it seems that unwinding from normal functions still
> >>> works as intended.
> >>
> >> Yes, I tried it on my end as well and it does work. What I was worried
> >> about is that we need to adjust the LR value in some cases.
> >>
> >> For aarch32 we need to remove the LSB, and for aarch64 we need to unmask
> >> the value of LR if it is signed through PAC. This is the reason why we
> >> have a custom dwarf2_prev_register implementation.
> >
> > Is the aarch64-tdep code used for aarch32 tracee as well?
> >
>
> The aarch64 code is not shared by aarch32 (that would be arm-tdep.c). I
> just mentioned it because both arm-tdep.c and aarch64-tdep.c use the
> same mechanism for unwinding PC. That is, they return LR instead.

OK, that makes sense.

>
> >> gcc and clang emit the return address column as r30. If we don't specify
> >> any initialization rule for PC, then the dwarf2 unwinder will go through
> >> the route of fetching LR and adjusting the values.
> >>
> >> On the other hand, if PC is available in a CFI column, then the dwarf
> >> unwinder won't call the custom hook and will instead proceed to
> >> calculate PC from the CFI expression at that column, which should give
> >> the result that you want.
> >
> > Right, and I guess that means that the custom unwind rule needs to
> > take the pointer authentication into account?
> > (I've never used any machines with that enabled so I'm not 100% how
> > it's supposed to work....)
>
> It does need to take that into account. But using a custom function for
> unwinding PC means we will not have access to
>
> >
> >> We can't initialize PC to DWARF2_FRAME_REG_RA though (not yet, anyway),
> >> as that would default to mapping PC to the return address column. For
> >> gcc and clang, this would be x30. That would give GDB no chance of
> >> adjusting the LR values if required (for PAC-signed LR values).
> >
> > Just so that I understand, if LR is signed, then upon return it should
> > still have the signed value. (Or in the case a different register is
> > used for return address, `ret xn` would not change xn value)
> > Only the resulting PC should have the signed part masked off. That's
> > why the unwinding of LR is correct without any post-processing logic
> > as-is but the unwinding of PC needs to be treated specially. >
>
> That's correct.
>
> >> It would be nice to have a testcase for this, alongside your patch, to
> >> make sure GDB is always doing the correct unwinding.
> >
> > The part for returning the name of PC is easy. I assume the test would
> > be mainly throwing in a aarch64-xxx.{c,exp} and to check if explicitly
> > specifying the PC in the unwind info result in valid unwinding. I'll
> > try look for some but any examples that I can follow?
>
> I think gdb/testsuite/gdb.arch/arm-disp-step.{S,exp} is a good example.
> Basically, some .S file that contains some random instructions and CFI
> directives that we single-step over and check if GDB is following the
> values of the registers correctly. The .S file gives us more control
> over those CFI directives.

By `.c` I actually mean using some inline assembly. But if .S is
supported it is indeed much easier to use.

> > This should fix the case where PC is explicitly specified but wont fix
> > the case where a return column is specified. Handling that requires a
> > different way to post-processing the PC.
>
> Correct. That is a separate bug and I'll need to investigate that.
>
> Thanks!
> Luis