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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
