Page MenuHomePhabricator

[lldb] Fix SLEB128 decoding
ClosedPublic

Authored by jankratochvil on Jun 3 2020, 2:16 PM.

Details

Summary

Bug 46181 shows SLEB128 0xED9A924C00011151 decoded as 0xffffffff80011151.

Diff Detail

Event Timeline

jankratochvil created this revision.Jun 3 2020, 2:16 PM
jankratochvil retitled this revision from Fix SLEB128 decoding to [lldb] Fix SLEB128 decoding.Jun 3 2020, 2:17 PM

Greg & Pavel might have opinions on this patch. I'm not qualified to review it.

Seems like it would be easier to add a unit test to lldb/unittests/Utility/DataExtractorTest.cpp. Then you can verify any edge case without having to make DWARF in a .s file?

shafik added a subscriber: shafik.Jun 3 2020, 7:57 PM

I would also like to see a unit test, unless the DWARF test somehow covers as aspect that a unit test would and even in that case the unit test is still important.

labath added a comment.Jun 4 2020, 6:27 AM

The fix is good (thanks for tracking that down), but the test is way to complicated for what the fix does. A generic "can we show the DW_AT_const_value of a global variable" test might be useful -- I don't believe we have anything exactly like that right now -- but it ought to be reduced as it contains a bunch of unrelated/unneeded stuff. However, that's strictly optional..

jankratochvil planned changes to this revision.Jun 4 2020, 6:41 AM

Added unit test, simplified the .s test.

labath accepted this revision.Jun 4 2020, 9:25 AM

Thanks. The fix and the unittest look good. I have a bunch of comments on the .s file. As I note in the inline comments, I don't think that having a test like that for specifically handling sleb encoding has value. But if we rephrase it to test printing of "const_values" then I think it is ok. I also note some additional simplifications opportunities.

PS: You can still just drop the test if you think that's too much hassle. :)

lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s
1 ↗(On Diff #268489)

For me, the interesting aspect of this test is that we have a very targeted test for the handling of const_values of variables. The fact that it uses a sleb128 (much less a wide negative sleb128) is not important here as that can be more easily tested elsewhere. So, I'd rename this test to something like DW_TAG_variable-const_value, and rephrase this comment to reflect that.

5 ↗(On Diff #268489)

I don't think -g -dwarf-version=5 is needed here.

6 ↗(On Diff #268489)

It's kinda weird (though very useful for tests) but lldb can open .o files too. You should be able to drop the linker and then the main function too.

11–13 ↗(On Diff #268489)

At this point the file is far enough from the original, that this and the "patched" lines are not useful.

24–28 ↗(On Diff #268489)

The offset comments aren't correct anymore. Just delete them. (I usually also inline my strings via DW_FORM_string forms, but you don't need to do that if you don't want to).

84 ↗(On Diff #268489)

.sleb128 0xed9a924c00011151

lldb/unittests/Utility/DataExtractorTest.cpp
270

I'm not sure how useful it is to have both big and little versions of these. If you think they add value, you can keep them, but I'd personally just use one (maybe the big one as it's more likely to flush out errors).

292

Should this be uint64_t ?

This revision is now accepted and ready to land.Jun 4 2020, 9:25 AM
jankratochvil marked 12 inline comments as done.Jun 4 2020, 10:35 AM

PS: You can still just drop the test if you think that's too much hassle. :)

It is sure nice to learn more the LLDB high standards, thanks for the review.

lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s
6 ↗(On Diff #268489)

I admit it works with .o now but when I was testing it before it did not (despite GDB could open such .o file).

11–13 ↗(On Diff #268489)

I disagree, this is a minimal C code to produce DWARF containing DW_TAG_variable + DW_AT_const_value. I have reworded the comment.

24–28 ↗(On Diff #268489)

I just do not think it is worth the development time to make even testcases so optimal but OK.

lldb/unittests/Utility/DataExtractorTest.cpp
270

I have kept them, what if someone makes there some endianity dependency, dunno.

292

Yes, thanks.

jankratochvil marked 5 inline comments as done.Jun 4 2020, 10:40 AM
jankratochvil added inline comments.
lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s
6 ↗(On Diff #268489)

I have thus removed the REQUIRES: lld.

This revision was automatically updated to reflect the committed changes.

Hi Jan, I noticed our sanitizer bot started getting failures in

Failing Tests (3):

lldb-api :: commands/expression/unwind_expression/TestUnwindExpression.py
lldb-unit :: Utility/./UtilityTests/DataExtractorTest.GetSLEB128_bit63
lldb-shell :: SymbolFile/DWARF/DW_TAG_variable-DW_AT_const_value.s

/Users/buildslave/jenkins/workspace/lldb-cmake-sanitized/llvm-project/lldb/source/Utility/DataExtractor.cpp:934:17: runtime error: negation of -9223372036854775808 cannot be represented in type 'int64_t' (aka 'long long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/buildslave/jenkins/workspace/lldb-cmake-sanitized/llvm-project/lldb/source/Utility/DataExtractor.cpp:934:17 in
http://green.lab.llvm.org/green/blue/organizations/jenkins/lldb-cmake-sanitized/detail/lldb-cmake-sanitized/1509/pipeline

TestUnwindExpression might be unrelated, but it looks like DataExtractorTest.GetSLEB128_bit63 and DW_TAG_variable-DW_AT_const_value.s are hitting this ubsan error after this commit.

lldb-unit :: Utility/./UtilityTests/DataExtractorTest.GetSLEB128_bit63
lldb-shell :: SymbolFile/DWARF/DW_TAG_variable-DW_AT_const_value.s

That should be fixed by rG846909e2ab0b, I will check the sanitized buildbot, sorry I did check only other buildbots.

labath added a comment.Jun 8 2020, 2:17 AM

PS: You can still just drop the test if you think that's too much hassle. :)

It is sure nice to learn more the LLDB high standards, thanks for the review.

Just to clarify. These are "standards" that I try to set for my self -- I think it's important to be able to quickly ascertain what a test does, and needing to skip over irreletant attributes or follow indirect references makes that hard (DWARF v5 makes this particularly bad as it introduces a lot more nonsymbolic reference via all DW_FORM_***x encodings). Ideally, then it would not be necessary to put a comment saying "this file declares a single long variable with the value 47" because that would be sufficiently obvious without that. Right now, we're pretty far from that, but there's a GSoC project http://lists.llvm.org/pipermail/llvm-dev/2020-May/141690.html, which I'm hoping will get us a step towards that.

The reason I chose to do that here is because I you seemed very motivated to get that test in, and I considered it very optional and the only meaning I could see in it was if it was polished very well. (After the first round of comments, I was expecting you would just delete that test). I do not insist on this level of reduction in most tests -- I am usually happy that there is a test in the first place.