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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

I'm missing some context here I think.

I assume the use case is that, for example, we don't want to be adding a bunch of WASM specific DWARF operations to the generic operations. If those can live in a WASM (the "vendor") plugin then we know where to look and they can use any vendor specific knowledge to do their job.

Along the right lines? I think I saw some talk about this on a previous diff but don't remember now.



================
Comment at: lldb/include/lldb/Target/Platform.h:959
                           const FileSpecList *module_search_paths_ptr);
-
 private:
----------------
stray 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);
 }
----------------
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).


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:343
+  ParseVendorDWARFOpcode(uint8_t op,
+                             const lldb_private::DataExtractor &opcodes,
+                             lldb::offset_t &offset,
----------------
Seems like clang-format missed a bit here.


================
Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:571
+    }
+
+    // Report the skipped distance:
----------------
Is there not 3 more arguments to read off here? The `0x00 0x00 0x00` from above.


================
Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:751-765
+  // Test that expression extensions can be evaluated, for example
+  // DW_OP_WASM_location which is not currently handled by DWARFExpression:
+  // DW_OP_WASM_location WASM_GLOBAL:0x03 index:u32
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_WASM_location, 0x03, 0x04, 0x00, 0x00,
+                                 0x00, DW_OP_stack_value},
+                                dwo_module_sp, dwo_dwarf_unit),
+                       llvm::HasValue(GetScalar(32, 42, false)));
----------------
Can you dedeupe this section of both tests? Appears to be the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137247