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