[PATCH] D143463: [X86] Use the CFA when appropriate for better variable locations around calls.

[Scott Linder via Phabricator via llvm-commits]

scott.linder accepted this revision as: scott.linder.
scott.linder added a comment.
This revision is now accepted and ready to land.

In D143463#4263367 <https://reviews.llvm.org/D143463#4263367>, @khuey wrote:

> In D143463#4262748 <https://reviews.llvm.org/D143463#4262748>, @scott.linder wrote:
>
>> Very reasonable, but if GCC is already using the CFA unconditionally it seems likely that implies a pretty good support base?
>
> Indeed.
>
>> I meant to propose another middle-ground, where we use the CFA whenever `!hasFP`, which (IIUC) is a precondition for x86 ever adjusting the CFA. It would mean we use the CFA more than with the heuristic, but still not unconditionally.
>
> !hasFP is a precondition for x86 emitting CFA offset adjustment directives solely because if there is a frame pointer LLVM simply defines the CFA to be the frame pointer. And then the frame pointer remains the same regardless of adjustments to %rsp, so the CFA doesn't need to be adjusted simultaneously. See the `createDefCfaRegister` call inside `X86FrameLowering::emitPrologue` (which is gated on, at the top level, HasFP).
>
>> I think I am mostly concerned with the statefulness of the heuristic, coupled with the fact that it seems to not be completely contained. The majority of it is handled by `X86FrameLowering`, but there is at least the one instance where it has to be explicitly managed in `X86MCInstLower`. If others are not as concerned with this I'm happy to concede on it, but it is the main concern I have.
>
> The not-being-contained is just because `X86::MOVPC32r` is really weird.

OK, I may have just been overly concerned about this; my familiarity with the X86 target in LLVM is very limited.

>> I definitely understand the complexity in `DwarfExpression::addMachineRegExpression` makes any change very difficult. I am happy with continuing to apply the offset to the CFA in the frame base, and if some other brave soul wants to change that in a future patch they won't be any worse off.
>
> Yeah I don't really want to do that here but I agree that folding the offset between the CFA and the stack pointer into the variable expressions rather than leaving it in the DW_AT_frame_base expression is the ideal end point.
>
>> My current leaning is to change to using the CFA unconditionally, but offset to maintain the same frame base value. It will result in a slightly larger expression in the default case, but I would be much more confident that the result is always correct.
>
> So, to be clear, you're learning towards using a CFA-based DW_AT_frame_base in all three of these scenarios?
>
> 1. There is a CFA, and LLVM is currently using %rsp as the DW_AT_frame_base, and %rsp changes throughout the function.
> 2. There is a CFA, and LLVM is currently using %rsp as the DW_AT_frame_base, and %rsp remains constant (after the prologue).
> 3. There is a CFA, and LLVM is currently using %rbp as the DW_AT_frame_base.
>
> Where using the CFA for 1 is what this patch as originally proposed does, and 1 + 2 is the "middle ground" you suggested previously, as I understand it.
>
> I think using the CFA for 1 + 2 + 3 makes more sense than doing just 1 + 2, so I think we more or less agree here.

I do think my leaning is towards using the CFA for 1 + 2 + 3, but I also appreciate that your patch as-is addresses a real bug that affects debug-info correctness. I am OK with accepting your patch with a `FIXME` or `TODO` describing the desire to eventually define the frame base as equal to the CFA.

I would just ask that you give others at least another day to respond. I recognize it has already been months, but one more day would make me a little less concerned that I missed something obvious that the more experienced LLVM debug-info devs would spot :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143463/new/

https://reviews.llvm.org/D143463