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

[Aleksandr Urakov via Phabricator via lldb-commits]

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

Hi Jason, thanks for the review!

Initially, the reason of these changes was to reuse the code that works with `eh_frame`, because on Windows we have a very similar compiler generated info designed to help with unwinding stacks during exception handling. That's why I have chosen `DWARFCallFrameInfo` and have extracted its interface (`I` in `ICallFrameInfo` stands for `Interface`, but I've renamed it to `CallFrameInfo` in the last patch, because this notation doesn't look common in LLVM).

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.

To resolve the situation we can:

- use something like a `dynamic_cast` to `ObjectFilePECOFF` in `UnwindTable` and create `PECallFrameInfo` with it. But in this case `lldbSymbol` becomes dependent on `lldbPluginObjectFilePECOFF` (and we get a circular dependency);
- extend `ObjectFile`'s interface to make it possible for `PECallFrameInfo` to work with required PE abstractions. In this case we can just create `PECallFrameInfo` in `UnwindTable` like we do with `DWARFCallFrameInfo`, but it adds weird operations to `ObjectFile`'s interface, which are not related to other object file formats;
- allow `ObjectFile` to create its own unwind infos by himself.

I've found the last idea good, and decided to redo things in this way (because `DWARFCallFrameInfo` etc. are working over object files too). But you are right, it also has a negative impact on `ObjectFile`: it becomes knowing of the things it shouldn't. So in the last patch I have left only a most abstract method, which only allows to get some unwind info from an object file, and introduced corresponding entities in `UnwindTable` and `FuncUnwinders`. I think it can be a good start for moving in the direction that Greg suggests.

What do you think of this?



================
Comment at: lldb/include/lldb/Symbol/DWARFCallFrameInfo.h:74
 
-  void ForEachFDEEntries(
-      const std::function<bool(lldb::addr_t, uint32_t, dw_offset_t)> &callback);
+  void ForEachEntries(const std::function<bool(lldb::addr_t, uint32_t)> &callback) override;
 
----------------
amccarth wrote:
> I find the name `ForEachEntries` confusing.  I know this is a leftover from the original code that you're modifying, but I wonder if it can get a better name.  In particular, I don't know why it's plural, so I'd expect `ForEachEntry`, but even that is pretty vague.  I realize `FDE` is DWARF-specific, but at least it gives a clue as to what type of entries are involved.
I just have removed it from the new version.


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp:541
+const RuntimeFunction *PECallFrameInfo::FindRuntimeFunctionItersectsWithRange(
+    const AddressRange &range) const {
+  uint32_t rva = m_object_file.GetRVA(range.GetBaseAddress());
----------------
amccarth wrote:
> Isn't it possible for more than one RuntimeFunction to intersect with an address range?  In normal use, it might not happen because it's being called with constrained ranges, so maybe it's nothing to worry about.  I suppose if the range were too large and it encompassed several functions, returning any one of them is acceptable.
Yes, it is possible. The problem here is that `FuncUnwinders` requests the plan with an `AddressRange`, not with an `Address`, and the information about  the original requested address is lost to that moment. The required `AddressRange` is calculated in `UnwindTable::GetAddressRange`, that's why the order of querying unwind infos is important in that function. I think that this logic requires some rework, but it seems that it is not a good idea to do it in this patch.


================
Comment at: lldb/source/Symbol/ObjectFile.cpp:687
+  return std::make_unique<DWARFCallFrameInfo>(*this, sect,
+                                              DWARFCallFrameInfo::EH);
+}
----------------
amccarth wrote:
> It seems a bit weird for DWARF-specific code to be here, when there are ObjectFile plugins for PECOFF, ELF, and Mach-O.  Obviously the PECOFFCallFrameInfo is instantiated in the PECOFF plugin.  The ELF and Mach-O versions instantiate DWARFCallFrameInfos.  Does the generic ObjectFile need to do the same?
I just have made a default implementation to avoid duplication of the code in ELF and Mach-O plugins, but I agree, it looks weird here. It was removed in the new implementation.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67347