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

[Philip Pfaffe via Phabricator via lldb-commits]

pfaffe added a comment.

Yes, you got the context right! Although technically speaking this change isn't limited to wasm, plugins can implement any vendor extension on top of this change.



================
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:
> 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? 


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:343
+  ParseVendorDWARFOpcode(uint8_t op,
+                             const lldb_private::DataExtractor &opcodes,
+                             lldb::offset_t &offset,
----------------
DavidSpickett wrote:
> Seems like clang-format missed a bit here.
Curious, I would have assumed the linter at the very least would have caught that.


================
Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:571
+    }
+
+    // Report the skipped distance:
----------------
DavidSpickett wrote:
> Is there not 3 more arguments to read off here? The `0x00 0x00 0x00` from above.
No, the second argument to this opcode is of type uint32_t, the zeros are the last three bytes of that. Should I maybe use a simpler/fake opcode? Might be dangerous if that fake opcode ever got occupied/implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137247