Page MenuHomePhabricator

Fix how ValueObjectVariable handles DW_AT_const_value when the DWARFExpression holds the data that represents a constant value
ClosedPublic

Authored by shafik on Aug 20 2020, 11:22 AM.

Details

Summary

In some cases when we have a DW_AT_const_value and the data can be found in the DWARFExpression then ValueObjectVariable does not handle it properly and we end up with an extracting data from value failed error:

(lldb) target var constant
(U) constant = {
  raw = <extracting data from value failed>

   = (a = <extracting data from value failed>, b = <extracting data from value failed>, c = <extracting data from value failed>, d = <extracting data from value failed>, e = <extracting data from value failed>, f = <extracting data from value failed>)
}

This should fix that case.

The test is a very stripped down assembly file since reproducing this relies on the results of compiling with -O1 which may not be stable over time.

Diff Detail

Event Timeline

shafik requested review of this revision.Aug 20 2020, 11:22 AM
shafik created this revision.

The code looks fine, I think the test needs an extra REQUIRES.

lldb/source/Core/ValueObjectVariable.cpp
137

I guess this looks reasonable.

lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
3

I think this is missing a REQUIRES: line that checks for an x86 target?

12

This sentence is complicated to parse.

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.

lldb/source/Core/ValueObjectVariable.cpp
136

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.

lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
12

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.

shafik marked an inline comment as done.Aug 21 2020, 6:47 AM
shafik added inline comments.
lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
3

I did not use that in D85376, is there something different in this case?

labath added inline comments.Aug 21 2020, 9:49 AM
lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
3

All tests using x86 assembly technically require that, but I don't believe we have a bot which is configured to build without that target, so nothing enforces that. I'm pretty sure there are other tests that need this stanza, but don't have it.

@shafik would you mind preparing a separate commit that add the REQUIRES everywhere it is needed, or if there are many, create an x86 subdir with an implicit requirement (like we do in the llvm/test/debuginfo tests)?
If it weren't for Rosetta, these would break on Apple Silicon machines, for example.

aprantl accepted this revision.Aug 21 2020, 1:09 PM
This revision is now accepted and ready to land.Aug 21 2020, 1:09 PM
shafik updated this revision to Diff 287482.Aug 24 2020, 1:42 PM
shafik marked 5 inline comments as done.

-Add REQUIRES x86

  • Fix-up the comments in the test
shafik added inline comments.Aug 24 2020, 2:14 PM
lldb/source/Core/ValueObjectVariable.cpp
136

I see what you are saying, let me verify this in a separate PR and if that bears out then we can remove this check.

Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2020, 3:17 PM