[Lldb-commits] [PATCH] D86311: Fix how ValueObjectVariable handles DW_AT_const_value when the DWARFExpression holds the data that represents a constant value

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

LGTM, modulo the defensive check. Incidentally, I was just looking at a DW_AT_const_value bug, but I think it's a different issue than this one.



================
Comment at: lldb/source/Core/ValueObjectVariable.cpp:136
+    if (expr.GetExpressionData(m_data)) {
+       if (m_data.GetDataStart() && m_data.GetByteSize())
+        m_value.SetBytes(m_data.GetDataStart(), m_data.GetByteSize());
----------------
I doubt this check is necessary -- surely we should be able to rely on `GetExpressionData` setting the data extractor to something reasonable if it returns success.

If anything, it would be nice to make a test with an empty DW_AT_const_value, which checks that lldb does something reasonable. It shouldn't that hard -- copy the DW_TAG_variable abbreviation, change DW_AT_const_value to DW_FORM_block, and make a new variable DIE with an empty block.


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s:11
+# This is testing when how ValueObjectVariable handles the case where the
+# DWARFExpression holds the data that represents a constant value.
+# Compile at -O1 allows us to capture this case. Below is the code used
----------------
aprantl wrote:
> This sentence is complicated to parse.
It may be enough to say "Check that we're able to display variables whose value is given by DW_AT_const_value" -- the relationship between (or even existance) of ValueObjectVariable and DWARFExpression may change, but the test case is sufficiently generic to remain valid. Though, given the test name, even that sentence is pretty redundant.


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

https://reviews.llvm.org/D86311