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

Greg Clayton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Mar 20 08:38:12 PDT 2019


clayborg 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) {
----------------
zturner wrote:
> 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.
remove "mapped_dwarf_data" arg


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:34-38
+  if (mapped_dwarf_data.hasValue()) {
+    extractor->SetData(*mapped_dwarf_data, section_sp->GetOffset(),
+                       section_sp->GetFileSize());
+    return extractor.getPointer();
+  }
----------------
remove


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:46-49
+void DWARFContext::SetDwarfData(const DWARFDataExtractor &dwarf) {
+  m_dwarf_data = dwarf;
+}
+
----------------
zturner wrote:
> 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.
remove


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:50
+
+const DWARFDataExtractor *DWARFContext::getOrLoadArangesData() {
+  return LoadOrGetSection(m_module, m_dwarf_data, eSectionTypeDWARFDebugAranges,
----------------
rename to "getArangesData()"? Would it be better to return an Optional<DWARFDataExtractor>?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:21
+  Module &m_module;
+  llvm::Optional<DWARFDataExtractor> m_dwarf_data;
+
----------------
clayborg wrote:
> What does m_dwarf_data contain? The entire file? Just the __DWARF segment for mach-o?
Remove this now that we don't need it since Section::GetSectionData() will do the right thing already.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:28
+
+  void SetDwarfData(const DWARFDataExtractor &dwarf);
+
----------------
zturner wrote:
> 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.
We can get rid of this after thinking about this.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:404-407
+    if (section) {
       m_obj_file->ReadSectionData(section, m_dwarf_data);
+      m_dwarf_context.SetDwarfData(m_dwarf_data);
+    }
----------------
Get rid of m_dwarf_data now that we don't need it? That will make subsequent patches easier.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:461
 
+  lldb_private::DWARFContext m_dwarf_context;
   lldb_private::DWARFDataExtractor m_dwarf_data;
----------------
"m_dwarf_ctx" to keep it shorter? Or just "m_ctx"?


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

https://reviews.llvm.org/D59562





More information about the lldb-commits mailing list