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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
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) {
----------------
is "mapped_dwarf_data" actually the entire file mmap'ed? If so rename this to "mapped_file_data"?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:40
+
+  module.GetObjectFile()->ReadSectionData(section_sp.get(), *extractor);
+  return extractor.getPointer();
----------------
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?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:46-49
+void DWARFContext::SetDwarfData(const DWARFDataExtractor &dwarf) {
+  m_dwarf_data = dwarf;
+}
+
----------------
Again, is this the entire file that is mapped?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:21
+  Module &m_module;
+  llvm::Optional<DWARFDataExtractor> m_dwarf_data;
+
----------------
What does m_dwarf_data contain? The entire file? Just the __DWARF segment for mach-o?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:28
+
+  void SetDwarfData(const DWARFDataExtractor &dwarf);
+
----------------
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.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:41
+  void SetDwarfData(SymbolFileDWARF *dwarf2Data,
+                    lldb_private::DWARFContext *dwarf_context);
 
----------------
Store as a reference maybe? We must assume that the DWARFContext will be alive as long as this class is around. We would need to supply this in the constructor then. Is that possible?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:70
   SymbolFileDWARF *m_dwarf2Data;
+  lldb_private::DWARFContext *m_dwarf_context;
   CompileUnitColl m_compile_units;
----------------
Be nice if this can be a reference and be set in the ctor


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:690-691
                        static_cast<void *>(this));
     if (get_debug_info_data().GetByteSize() > 0) {
       m_info.reset(new DWARFDebugInfo());
       if (m_info) {
----------------
```
m_info.reset(new DWARFDebugInfo(m_dwarf_context));
```


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:693
       if (m_info) {
-        m_info->SetDwarfData(this);
+        m_info->SetDwarfData(this, &m_dwarf_context);
       }
----------------
revert


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

https://reviews.llvm.org/D59562