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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
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);
 }
----------------
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)


================
Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:571
+    }
+
+    // Report the skipped distance:
----------------
pfaffe wrote:
> 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.
It was the comment misleading me:
```
// Called with "arguments" 0x03, 0x04, 0x00, 0x00, 0x00
```

So I'm thinking there is an operation that is called with arguments 3, 4, 0, 0, 0.

Just reword the comment and it'll be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137247