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

Thanks for replying Jason.

I think having the augmentation string specify its validity would be great, though I am somewhat sceptical of being able to push that idea through -- given that eh_frame needs to be understood by a lot of consumers and is actually critical for the correctness of some applications (those using exceptions), making changes to it seems to be hard -- that's why it's still stuck at version 1 of debug_frame.

That said, given that current compilers do emit prologue/epilogue unwind info correctly, I'm not sure that we need the epilogue augmentation code, without additional header information. Given all of this discussion I'll try to find some time to do a detailed investigation of the prologue/epilogue state in various compiler versions (similar to the DW_AT_low_pc analysis in D78489 <https://reviews.llvm.org/D78489>), and see what comes out of it.

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. My flow of thought which led to starting off with this patch was:

- I knew that instruction emulation is broken, but the very first plan we try is the "eh_frame augmented" plan.
- It seemed like the augmenter should also be able to handle this function (and it's job should be even easier). So I tried to find why that doesn't kick in.
- While stepping through the code, I found this comment (UnwindAssembly-x86.cpp:127), which seems to imply that we don't need to augment it at all. It stop short of expliticly saying that we should actually use that plan, but that's seems implied to me. (it also makes sense -- the augmenter is there to add the epilogue instructions, and we already have them, so there's nothing to add..)
- So, I figured I'd first fix it so that it actually does what it says it does.

>From your comment, it's not clear to me whether you are ok with this patch, or if you want to do something differently.

BTW, the call-next-instruction trick is still used, but not by any gcc version available on godbolt.org -- clang uses this sequence on i386 PIC. However, since clang-3.8, the cfi instruction properly describe how to unwind out of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82378