[PATCH] D143463: [X86] Use the CFA as the DWARF frame base for better variable locations around calls.

[Kyle Huey via Phabricator via llvm-commits]

khuey added a comment.

So this is actually a case of what I said earlier about there possibly being old tools out there that don't know how to cope with CFI. Turns out one of them is this "frame diagnose" feature in lldb.

Based on code inspection what I think is happening is that we're getting to this point https://github.com/llvm/llvm-project/blob/d9610b4a56c532614545eef5995362e99b776535/lldb/source/Expression/DWARFExpression.cpp#L2676. DWARFExpression::MatchesOperand takes the operand from a crashing instruction (e.g. [rbp + 42]) and tries to match that up with the locations of the various variables provided in the DWARF. It does this symbolically, looking for exactly two forms of DWARF expressions in variable locations, DW_OP_regN/x and DW_OP_fbreg <offset> where DW_AT_frame_base is itself DW_OP_regN/x. Because I changed DW_AT_frame_base to be DW_OP_call_frame_cfa DW_OP_consts <offset> DW_OP_plus, nothing on the stack is recognized anymore. There's no code here that knows how to deal with DW_OP_call_frame_cfa.

I don't see any easy way to fix this. The CFA is only available at this point in value form (via StackFrame's StackID m_cfa). Despite the comments, StackFrame::GetFrameBaseExpression really does return the DW_AT_frame_base expression, not the CFA (the author of the comment describing the function seems not to have understood that the frame base and the canonical frame address are not the same thing). With the symbolic form of the CFA available we could recognize the sequence DW_AT_location = DW_OP_fbreg <offset1>, DW_AT_frame_base = DW_OP_call_frame_cfa DW_OP_consts <offset2> DW_OP_plus, CFA = rXX + offset3 and match it to e.g. mov [%rXX + offset4], %rax where offset4 = offset1 + offset2 + offset3, but that would require a bunch of work in lldb to plumb the symbolic form of the CFA up out of the unwinding layer to somewhere where it's available to CommandObjectFrameDiagnose.

Without that, the main alternative I see is to revert back from using the CFA unconditionally to only using it in the cases where it's necessary for correctness. Then most functions out there will continue to have frame pointer expressions that fit the format "frame diagnose" is expecting, and the only functions it won't be able to handle are the ones that currently have wrong locations anyways.

As an aside, is this `frame diagnose` feature actually used? I've never heard of it, and some cursory googling (e.g. '"frame diagnose" lldb') suggests nobody else has either. The code looks like it's been basically untouched since it was written in 2016. Is there someone who can say it's ok to break it? It's already not working with any code gcc produced in the last decade or so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143463