labath added a comment.
In D67347#1664640 <https://reviews.llvm.org/D67347#1664640>, @aleksandr.urakov
wrote:
> It's a good point, thank you! I had the same thoughts when I done it, but I'm
> not completely sure. The problem is that an object file can't be completely
> responsible for choosing an unwind plan, because some plans are produced by
> symbol files (and an object file knows nothing about that, if I understand
> right). So we can move only a part of the plan-choosing-functionality from
> `FuncUnwinders` to `ObjectFile`s, but will it be better? In that case both
> `FuncUnwinders` and `ObjectFile`s will be responsible for managing plans,
> won't it add an additional complexity to the solution?
I spent a lot of time thinking about this when introducing the "breakpad"
unwind plans. My conclusion was too, that making ObjectFile solely responsible
for this is not a good idea, for the reasons that you already mention (and
others), but I haven't really found a solution that would be fully
satisfactory. Having the plans be managed by the symbol file is not that great
either, because a lot of unwind plans (instruction emulation) really don't have
anything to do with symbol files.
The solution that sounded most promising to me back then was making the unwind
plan providers pluggable. For instance, the Module (or probably the UnwindTable
contained within it) would contain a list of callbacks, which it would invoke
when it wanted to create an unwind plan for a given function. Then, anybody
could register itself as an unwind info provider. This format seems to be
naturally tied to the COFF object files, so the related code could live inside
ObjectFilePECOFF. The breakpad unwind info could be provided by
SymbolFileBreakpad. Things that are truly independent of any object or symbol
file format (like instruction emulation again) could be handled within the
UnwindTable by just pre-populating the list of callbacks.
This sounds pretty nice in theory, but what makes it harder in practice is that
there isn't just a single ultimate unwind plan for one function. There's the
synchronous/asynchronous distinction for one. And then, there are various
places throughout the code which reach for a specific unwind method to do some
special thing. Doing this is hard if the unwind method is abstracted behind an
abstract callback, and it's the reason I gave up on this idea, originally
However, maybe it would be good to resurrect it. The case for a callback
solution is stronger if we have two methods that would make use of it (this
stuff, and the breakpad format) than it was when we had just one. So, we could
port these two methods to the callback mechanism, and others could follow at
their own pace. I'd be happy to help with the breakpad side of things if needed.
All that said, I do think there is some degree of elegance in how you've done
things in this patch (though I am not a fan of the windows ISomething
convention). I've been thinking a lot about what will we do once we get to
implementing win64 unwinding, and I haven't though of this. Though, like I
already said, none of these solutions seems truly *right*, so I'd like to hear
what others think about this too.
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp:15
+static const T *TypedRead(const DataExtractor &data_extractor, offset_t
&offset, offset_t size = sizeof(T)) {
+ return static_cast<const T *>(data_extractor.GetData(&offset, size));
+}
----------------
It doesn't look like this will do the right thing in if host endianness differs
from the target. You should use GetMaxU64 instead. (or given that PE should be
always little-endian (right?), you could also ditch the DataExtractor and read
stuff by casting to llvm::support::ulittleXX_t. I believe that's how most of
PECOFF parsing in llvm works.)
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp:178
+
+ if (machine_reg >= sizeof machine_to_lldb_register / sizeof
+ machine_to_lldb_register[0])
----------------
llvm::array_lengthof
================
Comment at: lldb/source/Symbol/DWARFCallFrameInfo.cpp:454
+ Host::SystemLog(Host::eSystemLogError,
+ "error: Invalid fde/cie next "
+ "entry offset of 0x%x found in "
----------------
Too much clang format. Please make sure you only format the lines you modify.
================
Comment at: lldb/unittests/Symbol/CMakeLists.txt:17
lldbPluginSymbolFileDWARF
+ lldbPluginObjectFilePECOFF
lldbPluginSymbolFileSymtab
----------------
As the code you're testing lives in ObjectFilePECOFF, it would probably be
better to move this stuff to `unittests/ObjectFile/PECOFF` (new folder).
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67347/new/
https://reviews.llvm.org/D67347
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
