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

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I have a couple more questions and some renaming requests.



================
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;
 
----------------
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.


================
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());
----------------
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.


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h:46
+private:
+  const llvm::Win64EH::RuntimeFunction *FindRuntimeFunctionItersectsWithRange(
+      const lldb_private::AddressRange &range) const;
----------------
nit:  missing `n`: FindRuntimeFunctionI**n**tersectsWithRange


================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.h:50
+  ObjectFilePECOFF &m_object_file;
+  lldb_private::DataExtractor m_exception_data;
+};
----------------
`m_exception_data` is vague.  In the constructor, it's referred to as the exception directory, so perhaps `m_exception_dir` would be a bit more descriptive.


================
Comment at: lldb/source/Symbol/ObjectFile.cpp:687
+  return std::make_unique<DWARFCallFrameInfo>(*this, sect,
+                                              DWARFCallFrameInfo::EH);
+}
----------------
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?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D67347