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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to