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

On 5/11/22 14:26, Yichao Yu wrote:
> On Tue, May 10, 2022 at 10:49 AM Luis Machado <[email protected]> wrote:
>>
>> On 5/9/22 15:24, Yichao Yu wrote:
>>> On Mon, May 9, 2022 at 6:44 AM Luis Machado <[email protected]> wrote:
>>>>
>>>> On 5/6/22 17:30, Yichao Yu wrote:
>>>>> On Fri, May 6, 2022 at 12:11 PM Yichao Yu <[email protected]> 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.

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

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

> 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