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

Wed Sep 11 05:36:32 PDT 2019

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