[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Hi Aleksandr, nice job getting this working.  I read over the patch and had a couple of initial questions.

I don't understand the creation of the ICallFrameInfo base class.  All of the unwind info providers (things which parse their own format and output an UnwindPlan) are independent.  We could create a base class for these to give them an interface they must implement if that's helpful to something.  I don't understand how DWARFCallFrameInfo and the PE EH format are related; why is it closer than the CompactUnwindInfo or ArmUnwindInfo.  I don't understand what the "I" at the beginning of the base class name is indicating, but that's a minor point.

Ideally all details of the input sources are hidden behind the UnwindTable and FuncUnwinders objects in the Module. We obviously have places in generic code that look for a specific unwind source for something unusual.  One spot is in a section your patch modifies, in ObjectFileMachO when we don't have an authoritative source of information for function start addresses, we see if we have eh_frame information which gives us the start addresses for every function with eh information.

Most of the unwind formats are not wedded to a specific ObjectFile or SymbolFile format, so having them be separate from the OF/SF class is a good separation IMO.  For instance, CompactUnwindInfo is only used, today, on Darwin systems.  But it's a pretty clever format and I could see it being adopted on an ELF system.  Putting compact unwind parsed in ObjectFileMachO would prevent that kind of flexibility.

I agree with the general point that the FuncUnwinders class is messy, but that mess is mostly contained behind a simple API.  I'm not opposed to rethinking this -- plugins is an interesting idea -- but it doesn't bother me a lot given how little this code is typically worked on.

I probably read through the patch too quickly to understand it, but why is this different from other unwind info providers?  From the UnwindTable we have access to the ObjectFile and SymbolFile directly - I don't understand the motivation of that part of the change. I don't understand the move of the methods into the ObjectFile class - I don't object to it, but I'm missing the motivation for moving things there from the UnwindTable.

I guess it's best to start out with, the most basic: why isn't this a sources/Symbol/PEUnwindInfo.cpp standalone that UnwindTable instantiates and FuncUnwinders asks for UnwindPlans from.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67347