Re: [PATCH] Fix remaining inline/tailcall unwinding breakage for x86_64

[Luis Machado <luis.machado@linaro.org>]

On 4/25/20 12:53 PM, Tom de Vries wrote:
> On 25-04-2020 06:09, Luis Machado wrote:
>> Commit 5939967b355ba6a940887d19847b7893a4506067 fixed inline
>> frame unwinding breakage for some targets (aarch64, riscv, s390...)
>> but regressed a few amd64 testcases related to tailcalls.
>>
>> Given the following example situation...
>>
>> Frame #-1 - sentinel frame
>> Frame # 0 - inline frame
>> Frame # 1 - normal frame
>>
>> ... suppose we're at level #1 and call into dwarf2_tailcall_sniffer_first.
>>
>> We'll attempt to fetch PC, which used to be done via the gdbarch_unwind_pc call
>> (before 5939967b355ba6a940887d19847b7893a4506067), but now it is being handled
>> by the get_frame_register function.
>>
>> gdbarch_unwind_pc will attempt to use frame #1's cache to retrieve information
>> about the PC. Here's where different architectures behave differently.
>>
>> x86_64 will find a dwarf rule to retrieve PC from memory, at a CFA + offset
>> location. So the PC value is readily available and there is no need to
>> create a lazy value.
>>
>> For aarch64 (and others), GCC doesn't emit an explicit location for PC, so we
>> eventually will find that PC is DWARF2_FRAME_REG_UNSPECIFIED. This is known
>> and is handled by GDB by assuming GCC really meant DWARF2_FRAME_REG_SAME_VALUE.
>>
>> This means we'll attempt to fetch the register value from frame #0, via a call
>> to frame_unwind_got_register, which will trigger the creation of a lazy value
>> that requires a valid frame id for frame #0.
>>
>> We don't have a valid id for frame #0 yet, so we assert.
>>
>> Given the above, the following patch attempts to handle the situation without
>> being too hacky. We verify if the next frame is an inline frame and if its
>> frame id has been computed already. If it hasn't been computed yet, then we
>> use the safer get_frame_register function, otherwise we use the regular
>> gdbarch_unwind_pc hook.
>>
>> I've verified this makes both aarch64-linux and x86_64 happy testsuite-wise.
>>
> 
> Hi Luis,
> 
> thanks for working on this.
> 
> I've tested this patch on x86_64-linux and can confirm that this fixes
> all the regressions I saw.
> 
> I've reviewed the patch and it looks ok to me.
> 
> Please check this in, given that if fixes regressions.  If there are any
> comments from others to be addressed, that can still be done post-commit.
> 
> Thanks,
> - Tom
> 

Thanks for the quick review. Pushed now.