[PATCH] D66791: [lldb][ELF] Read symbols from .gnu_debugdata sect.

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1901
+  ArchSpec spec = m_gnuDebugDataObjectFile->GetArchitecture();
+  if (spec && m_gnuDebugDataObjectFile->SetModulesArchitecture(spec)) {
+    return m_gnuDebugDataObjectFile;
----------------
kwk wrote:
> labath wrote:
> > kwk wrote:
> > > labath wrote:
> > > > Are you sure you want to do that? Presumably, the architecture of the module was already set by the containing object file, and that file probably has a more correct understanding of what the right architecture is...
> > > Correct, the containing object file has a better understanding of things. Are you suggesting to do this then?
> > > 
> > > ```
> > > ArchSpec spec = GetArchitecture();
> > >   if (spec && m_gnuDebugDataObjectFile->SetModulesArchitecture(spec))
> > >     return m_gnuDebugDataObjectFile;
> > > ```
> > No, I think you should just delete this bit of code completely (as the containing object file will call `SetModulesArchitecture` on its own).
> Let me try if it works how you suggested it. No, it doesn't work as no breakpoint from the embedded .gnu_debugdata section will be hit anymore. This section was inspired by `ObjectFileELF::CreateInstance` as you suggested to look at once.
I'd be very surprised if this makes a difference as calling `SetModulesArchitecture` just invokes Module::SetArchitecture, which is actually a no-op if the module architecture has already been set.

However, I see now that SetArchitecture also acts as a check, which ensures that the architecture of the (nested) object file is compatible with the module architecture. This bit sounds like a useful check against someone accidentally (however unlikely) embedding a different object file into the gnu_debugdata section, so I guess we can keep that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791