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

[Luis Machado <luis.machado@arm.com>]

On 5/11/22 15:51, Luis Machado 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.
> 
>>>>
>>>> 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
> 

Sorry, this got truncated. With a custom function for unwinding PC, it 
means that we only invoke such function if CFI is not available for a PC 
column. If it is, then we follow the CFI rule but don't do any 
additional processing that would've taken place in the custom function.

Given the case of having a PC column is not so common, this should be OK 
for now.

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