[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Zachary Turner via Phabricator via lldb-commits]

zturner marked 5 inline comments as done.
zturner added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:17
+static const DWARFDataExtractor *LoadOrGetSection(
+    Module &module, const llvm::Optional<DWARFDataExtractor> &mapped_dwarf_data,
+    SectionType section_type, llvm::Optional<DWARFDataExtractor> &extractor) {
----------------
clayborg wrote:
> is "mapped_dwarf_data" actually the entire file mmap'ed? If so rename this to "mapped_file_data"?
It's the contents of the `__DWARF` segment on MachO, unless my reading of the old code is wrong.

In the old code, `SymbolFileDWARF` would try to read this segment, and if it existed set it to `SymbolFileDWARF::m_dwarf_data`.  Then, the `ReadCachedSectionData` function would first check if this member had data in it, and if so read the data from there instead of from the ObjectFile.

In this modified version of the patch, I call `SetDwarfData` with the contents of this section, then pass it through to this function so that we can perform the same logic.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:40
+
+  module.GetObjectFile()->ReadSectionData(section_sp.get(), *extractor);
+  return extractor.getPointer();
----------------
clayborg wrote:
> Use:
> 
> ```
>   lldb::offset_t Section::GetSectionData(DataExtractor &data);
> ```
> We might want to make the ObjectFile::ReadSectionData private so people don't use it and add a doxygen comment to use the above API in Section?
Yes, I like that better.  Thanks for pointing it out.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:46-49
+void DWARFContext::SetDwarfData(const DWARFDataExtractor &dwarf) {
+  m_dwarf_data = dwarf;
+}
+
----------------
clayborg wrote:
> Again, is this the entire file that is mapped?
As mentioned elsewhere, this is the contents of `__DWARF`, which I didn't understand the purpose of, so I was hesitant to make any drastic changes.  Perhaps you could clarify the significance of it now though and perhaps suggest an alternative approach to what we're doing here.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:28
+
+  void SetDwarfData(const DWARFDataExtractor &dwarf);
+
----------------
labath wrote:
> clayborg wrote:
> > labath wrote:
> > > Don't want to block progress here over this, but I just wanted to say that my impression was that this DwarfData business was a relict from the time when we re not mmapping the input object files. These days, we always mmap the input, and so I don't believe that should be necessary because ObjectFileMachO will automatically perform the DataBuffer slicing that you're doing here manually as a part of ReadSectionData.
> > > 
> > > Maybe one of the Apple folks could check whether this is really needed, because if it isn't, it will make your job easier here, as well as produce an API that is more similar to its llvm counterpart.
> > We don't always mmap. We mmap if the file is local. We read the entire file if the file is on a network drive. If we don't do this and we mmap a NFS file, and that mount goes away, we crash the next time we access a page that hasn't been paged in. So we don't know if stuff is actually mmap'ed or not.
> Yeah, I wasn't being fully correct here, but this distinction does not really matter here. Even if we don't mmap, we will end up with a full image of the file in memory, so anybody can slice any chunk of that file as he sees fit. Since ObjectFileMachO::ReadSectionData already implements this slicing, there's no advantage in slicing manually here.
The parameter here represents the content of the `__DWARF` data segment on MachO.  I don't really know what this is for or the implications of removing it, so for now I left it identical to the original code with NFC, and planned on addressing this in the future.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:70
   SymbolFileDWARF *m_dwarf2Data;
+  lldb_private::DWARFContext *m_dwarf_context;
   CompileUnitColl m_compile_units;
----------------
clayborg wrote:
> Be nice if this can be a reference and be set in the ctor
Fair enough, I kept it as a pointer for parity with `SymbolFileDWARF* m_dwarf2Data`, but a reference is definitely better.


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

https://reviews.llvm.org/D59562