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