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

Mon Sep 16 01:17:39 PDT 2019

labath added a comment.

Sorry for the delay, I was OOO. I haven't yet read through all of the discussion, but I wanted to plug into this part.

In D67347#1669877 <https://reviews.llvm.org/D67347#1669877>, @aleksandr.urakov wrote:

> But there is one more reason that makes it very difficult to just create `PECallFrameInfo` in `UnwindTable` like we do with `DWARFCallFrameInfo`: `PECallFrameInfo` is coupled very tight with `ObjectFilePECOFF` and can't work over plain `ObjectFile`. First of all, it uses several functions for work with relative virtual addresses (RVA), and they are implemented with the use of `ObjectFilePECOFF`'s private variable `m_image_base` (however, may be it is possible to implement these methods over the `ObjectFile` interface, I'm not sure about that). The second, it requires information about exception directory location, but it is an inner mechanics of `ObjectFilePECOFF`. So its relations with `ObjectFilePECOFF` are not the same as between `DWARFCallFrameInfo` and ELF or Mach-O.

PE m_image_base is already available through generic object file interfaces. You can access it as obj_file->GetBaseAddress().GetFileAddress(). This is the same way as the Breakpad symbol file plugin does it. As for the location of the exception directory, it sounds like it would not be too horrendous to have the PE plugin parse that and expose this bit as a special section (with it's own special section type etc.). Then, we could (hopefully) write the PE unwind parser in a way which does not depend on object file internals, similar to how the eh_frame parser does it.

That said, there are two reasons I am not 100% in favour of that idea:

- the current location of the unwind parsers (source/Symbol) seems pretty weird to me. The code in this folder already does a lot of stuff, and most of it has nothing to do with unwinding. So, shoving more unwinding code there seems suboptimal. If we go down that path, I would propose we create a new folder (source/Unwind ?) and put the unwind parsers there.
- even with a separate "unwind" folder, it seems slightly weird to have formats which are clearly specific to one {symbol|object}file be in a generic place. I am here mostly thinking of more "exotic" formats like breakpad. While it would be possible (and I am happy to do that) to refactor the breakpad parser to work in this way, in does not seem completely optimal to me. That's why I would still be in favour of some "plugin" mechanism, where unwind formats which are definitely tied to some kind of a file format be implemented there. Then the more generic formats could live in the generic place, and things like breakpad (and possibly PECOFF) could live inside their plugins.

(Overall, don't take my comments as a hard objection to anything. It's just an opinion that I am happy to be overridden on.)

================
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));
+}
----------------
aleksandr.urakov wrote:
> 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.
Ah sorry. I missed that part. This seems fine then.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67347