Bug 46181 shows SLEB128 0xED9A924C00011151 decoded as 0xffffffff80011151.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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?
Comment Actions
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.
Comment Actions
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..
Comment Actions
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 | ||
|---|---|---|
| 2 | 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. | |
| 6 | I don't think -g -dwarf-version=5 is needed here. | |
| 7 | 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. | |
| 12–14 | At this point the file is far enough from the original, that this and the "patched" lines are not useful. | |
| 25–29 | 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). | |
| 85 | .sleb128 0xed9a924c00011151 | |
| lldb/unittests/Utility/DataExtractorTest.cpp | ||
| 270 ↗ | (On Diff #268489) | 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 ↗ | (On Diff #268489) | Should this be uint64_t ? |
Comment Actions
It is sure nice to learn more the LLDB high standards, thanks for the review.
| lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s | ||
|---|---|---|
| 7 | I admit it works with .o now but when I was testing it before it did not (despite GDB could open such .o file). | |
| 12–14 | I disagree, this is a minimal C code to produce DWARF containing DW_TAG_variable + DW_AT_const_value. I have reworded the comment. | |
| 25–29 | 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 ↗ | (On Diff #268489) | I have kept them, what if someone makes there some endianity dependency, dunno. |
| 292 ↗ | (On Diff #268489) | Yes, thanks. |
| lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s | ||
|---|---|---|
| 7 | I have thus removed the REQUIRES: lld. | |
Comment Actions
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.
Comment Actions
That should be fixed by rG846909e2ab0b, I will check the sanitized buildbot, sorry I did check only other buildbots.
Comment Actions
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.
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.