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

Wed Jun 24 08:36:56 PDT 2020

labath added a comment.

In D82378#2109467 <https://reviews.llvm.org/D82378#2109467>, @jankratochvil wrote:

> In D82378#2109330 <https://reviews.llvm.org/D82378#2109330>, @labath wrote:
>
> > Now this situation is not actually handled by lldb's "augmenter", so the example somewhat shaky, but it does show that there are gaps in clang/llvm modelling of unwind info.
>
>
> Good to know but ... I do not see how is this `stdcall` unwinding bug related to this issue whether to use `.eh_frame` or not - `UnwindAssembly_x86::AugmentUnwindPlanFromCallSite` cannot verify any `.eh_frame` is 100% valid.

It's not related, at least not directly.  I was mainly responding to the claim that all (current) compilers produce asynchronous unwind tables. This example disproves that (well, one could claim that this is an asynchronous unwind table, but that it is an incorrect one, but I don't think that distinction is relevant).

And if there's one bug/incompleteness, there could very well be others, so I was also alluding to the fact that we might need to have some sort of an "augmentation framework" for the forseeable future. However, it could very well be the case that we no longer need to augment epilogue data, and if that's the case deleting the code for doing that would be great (but for that someone would need to investigate the epilogue behavior of different compilers more closely).

> 
> 
>> As for the DW_AT_producer idea, the main gotcha I see there is that eh/debug_frame is supposed to be usable even without the rest of the debug info.
> 
> Producer is also recorded in `.gnu.build.attributes` which is in the main binary (not in external `.debug` info):

Well... I don't think that's an improvement. :/ This section seems to be present only on some flavours of linux (my distro doesn't have it), so one cannot even apply it to linux universally, much less other operating systems.  So, I don't see the reason for changing the current detection algorithm. I think that a much more worthwhile use of time would be to check whether even need to detect something in the first place.

In D82378#2109924 <https://reviews.llvm.org/D82378#2109924>, @jankratochvil wrote:

> I do not understand why the testcase has **two** epilogues.  The `UnwindAssembly_x86::AugmentUnwindPlanFromCallSite()` code tests only beginning and end of CFI, it does not read anything in between.
>  The simplified testcase works the same for me: https://people.redhat.com/jkratoch/D82378.patch

True. The reason I did that was because in the single-epilogue case, we would unwind correctly using pretty much any method (e.g. the instruction emulation method, which was used before this patch). But you're right that that is not important for this patch. I'll use the simplified version.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82378