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

Thu Sep 12 01:11:58 PDT 2019

aleksandr.urakov marked 4 inline comments as done.
aleksandr.urakov added a comment.

Thanks all for taking a look!

In D67347#1666509 <https://reviews.llvm.org/D67347#1666509>, @amccarth wrote:

> This patch is pretty large.  It might be easier to break it up into a series of small steps to get there.  The bullet points in the CL description might be a good roadmap for such a break-up.  But let's first figure out the right abstraction points.

I'm open to this, just let me know.

================
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));
+}
----------------
labath wrote:
> 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.)
I believe that this code is ok: it actually doesn't read a pointer to data, it gets the data read from the file and interprets it like a structure of type `T` (`GetData` returns just a pointer to the data buffer). I think it's safe because `T` can be `RuntimeFunction`, `UnwindInfo` or `UnwindCode`, but all these types are already using `ulittleXX_t` in their definitions.

================
Comment at: lldb/unittests/Symbol/CMakeLists.txt:17
     lldbPluginSymbolFileDWARF
+    lldbPluginObjectFilePECOFF
     lldbPluginSymbolFileSymtab
----------------
labath wrote:
> As the code you're testing lives in ObjectFilePECOFF, it would probably be better to move this stuff to `unittests/ObjectFile/PECOFF` (new folder).
My bad, fixed, thank you!

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67347