[Lldb-commits] [PATCH] D82378: [lldb/Unwind] Use eh_frame plan directly when it doesn't need to be augmented

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D82378#2113865 <https://reviews.llvm.org/D82378#2113865>, @labath wrote:

> You're right that the instruction emulation not handling multiple epilogues is a bug. It's probably not about _all_ epilogues, only some special ones -- for example, the function where we ran into this was so simple it did not have any traditional prologue instructions and I suspect this is what threw the code off.
>
> I was planning to look into that separately.


I started looking into it now. The problem seems to be that the `pop %rcx` instruction is not treated as part of the epilogue -- the code only treats volatile register restores as a part of the epilogue. However, in this case, the `pop %rcx` is not there because the program needs to restore the register values -- it's there simply to undo the stack alignment that was done in the prologue to satisfy the ABI. This then causes us to incorrectly restore the "before-prologue" state when going past the first `ret`, and adjusting the stack twice:

  pushq %rax # align stack => CFA=rsp+16
  # some function call
  jcc 1f
  # some code
  pop %rcx # unalign stack => CFA=rsp+8
  retq # should reset CFA=rsp+16, but does +8 instead
  1: # more code, unwind info incorrect here
  pop %rcx # => CFA=rsp+0, should be +8
  retq

The simplest fix for that would be to treat _all_ pop instructions as the beginning of prologue (basically, move this <https://github.com/llvm/llvm-project/blob/808142876c10b52e7ee57cdc6dcf0acc5c97c1b7/lldb/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp#L1090> line out of the enclosing `if(non-volatile)` block), but I don't know whether that isn't too aggressive (it should be mostly ok, because current compilers prefer `movq ..., -N(%rsp)` instead of push/pop sequences, but still...). What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82378