[Lldb-commits] [PATCH] D137247: [lldb] Allow plugins to extend DWARF expression parsing for vendor extensions

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks ok, module some inline comments.



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2580
+                llvm::dyn_cast<SymbolFileDWARFDwo>(symbol_file)) {
+          symbol_file = &dwo_symbol_file->GetBaseSymbolFile();
+        }
----------------
For other methods, we just have the dwo class override the method and call the main file instead.


================
Comment at: lldb/source/Expression/DWARFExpressionList.cpp:93
   const DWARFExpression &expr = m_exprs.GetEntryRef(0).data;
-  return expr.ContainsThreadLocalStorage();
+  return expr.ContainsThreadLocalStorage(*m_dwarf_cu);
 }
----------------
DavidSpickett wrote:
> pfaffe wrote:
> > DavidSpickett wrote:
> > > I'm not sure that `m_dwarf_cu` is always non null. It might be in practice but for example one use ends up in `UpdateValueTypeFromLocationDescription` which checks that it is not null before use.
> > > 
> > > Maybe it is reasonable to assume it is never null (and if it is and you have the time, a preparatory patch to make it a ref would be great).
> > I think it's worth checking in general. Before https://reviews.llvm.org/D125509, the DWARFExpression used to be assiciated with its dwarf_cu. I was considering bringing that back, what do you think? 
> I think you should ask the author of that change :)
> 
> On the surface that change makes sense but I'm no expert. If that's the case then you'll just want to sprinkle some `if not nullptr` around wherever you use it.
> 
> (nullptrs are fine if there is a reason to use the null state)
DWARF expressions are used in contexts where there is no DWARF unit to speak of. Our use of DWARF expressions in PDB (the motivation behind that patch) is questionable at best, but DWARF expressions are also used in eh_frame for instance, which I think would be a good reason to keep DWARFExpression DWARFUnit-free. OTOH, some of the DWARF operations cannot be evaluated without a DWARFUnit, so.... I don't know...


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h:45
 
+  SymbolFileDWARF &GetBaseSymbolFile() const { return m_base_symbol_file; }
+
----------------
and then you don't need to make this public.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137247