[lldb] Fix SLEB128 decoding

[jankratochvil]

Summary

Bug 46181 shows SLEB128 0xED9A924C00011151 decoded as 0xffffffff80011151.

[davide]

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

[clayborg]

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]

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]

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]

Added unit test, simplified the .s test.

[labath]

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 ?

[jankratochvil]

In D81119#2073973, @labath wrote:

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]

lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s

6 ↗

(On Diff #268489)

I have thus removed the REQUIRES: lld.

[jasonmolenda]

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.

[jankratochvil]

In D81119#2074954, @jasonmolenda wrote:

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]

In D81119#2074109, @jankratochvil wrote:

In D81119#2073973, @labath wrote:

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.

lldb/source/Utility/DataExtractor.cpp

while (src < end) {
       result |= static_cast<int64_t>(byte & 0x7f) << shift;
       shift += 7;
       if ((byte & 0x80) == 0)
         break;
     }
 
     // Sign bit of byte is 2nd high order bit (0x40)
     if (shift < size && (byte & 0x40))
-      result |= -(1 << shift);
+      result |= -(static_cast<int64_t>(1) << shift);
 
     *offset_ptr += bytecount;
     return result;
   }
   return 0;
 }
 
 // Skips a ULEB128 number (signed or unsigned) from this object's data starting

lldb/test/Shell/SymbolFile/DWARF/DW_TAG_variable-DW_AT_const_value.s

+# This tests that lldb is able to print DW_TAG_variable using DW_AT_const_value.
+
+# REQUIRES: x86
+
+# RUN: llvm-mc -triple x86_64-unknown-linux-gnu %s -filetype=obj > %t.o
+# RUN: %lldb %t.o -o "p/x magic64" -o exit | FileCheck %s
+
+# CHECK: (const long) $0 = 0xed9a924c00011151
+
+# The DW_TAG_variable using DW_AT_const_value. can be produced from:
+# static const long magic64 = 0xed9a924c00011151;
+# int main(void) { return magic64; }
+
+  .text
+  .globl  main                    # -- Begin function main
+  .type main,@function
+main:                                   # @main
+.Lfunc_end0:
+  .size main, .Lfunc_end0-main
+                                        # -- End function
+  .section  .debug_abbrev,"",@progbits
+  .byte 1                       # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte 1                       # DW_CHILDREN_yes
+  .byte 37                      # DW_AT_producer
+  .byte 8                       # DW_FORM_string
+  .byte 19                      # DW_AT_language
+  .byte 5                       # DW_FORM_data2
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 2                       # Abbreviation Code
+  .byte 52                      # DW_TAG_variable
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 8                       # DW_FORM_string
+  .byte 73                      # DW_AT_type
+  .byte 19                      # DW_FORM_ref4
+  .byte 28                      # DW_AT_const_value
+# .byte 15                      # DW_FORM_udata
+  .byte 13                      # DW_FORM_sdata
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 3                       # Abbreviation Code
+  .byte 38                      # DW_TAG_const_type
+  .byte 0                       # DW_CHILDREN_no
+  .byte 73                      # DW_AT_type
+  .byte 19                      # DW_FORM_ref4
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 4                       # Abbreviation Code
+  .byte 36                      # DW_TAG_base_type
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 8                       # DW_FORM_string
+  .byte 62                      # DW_AT_encoding
+  .byte 11                      # DW_FORM_data1
+  .byte 11                      # DW_AT_byte_size
+  .byte 11                      # DW_FORM_data1
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 0                       # EOM(3)
+  .section  .debug_info,"",@progbits
+.Lcu_begin0:
+  .long .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+  .short  4                       # DWARF version number
+  .long .debug_abbrev           # Offset Into Abbrev. Section
+  .byte 8                       # Address Size (in bytes)
+  .byte 1                       # Abbrev [1] 0xb:0x61 DW_TAG_compile_unit
+  .asciz  "clang version 10.0.0"  # DW_AT_producer
+  .short  12                      # DW_AT_language
+  .byte 2                       # Abbrev [2] 0x2a:0x15 DW_TAG_variable
+  .asciz  "magic64"               # DW_AT_name
+  .long .Lconst                 # DW_AT_type
+        .sleb128 0xed9a924c00011151     # DW_AT_const_value
+.Lconst:
+  .byte 3                       # Abbrev [3] 0x3f:0x5 DW_TAG_const_type
+  .long .Lint64                 # DW_AT_type
+.Lint64:
+  .byte 4                       # Abbrev [4] 0x44:0x7 DW_TAG_base_type
+  .asciz  "long int"              # DW_AT_name
+  .byte 5                       # DW_AT_encoding
+  .byte 8                       # DW_AT_byte_size
+  .byte 0                       # End Of Children Mark
+.Ldebug_info_end0:

lldb/unittests/Utility/DataExtractorTest.cpp

TEST(DataExtractorTest, GetMaxU64_unchecked) {
   // Check with the maximum allowed byte size.
   offset = 0;
   EXPECT_EQ(0x0807060504030201U, LE.GetMaxU64_unchecked(&offset, 8));
   EXPECT_EQ(8U, offset);
   offset = 0;
   EXPECT_EQ(0x0102030405060708U, BE.GetMaxU64_unchecked(&offset, 8));
   EXPECT_EQ(8U, offset);
 }
+
+TEST(DataExtractorTest, GetSLEB128_bit63) {
+  uint8_t buffer[] = {0xff, 0x80, 0xff, 0x80, 0xff, 0x80, 0xff, 0x80, 0x7f};
+
+  DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+                   sizeof(void *));
+  DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *));
+
+  lldb::offset_t offset;
+
+  int64_t expected =
+      0b1111111100000001111111000000011111110000000111111100000001111111;
+  offset = 0;
+  EXPECT_EQ(expected, LE.GetSLEB128(&offset));
+  EXPECT_EQ(9U, offset);
+  offset = 0;
+  EXPECT_EQ(expected, BE.GetSLEB128(&offset));
+  EXPECT_EQ(9U, offset);
+}
+
+TEST(DataExtractorTest, GetULEB128_bit63) {
+  uint8_t buffer[] = {0xff, 0x80, 0xff, 0x80, 0xff, 0x80, 0xff, 0x80, 0x7f};
+
+  DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+                   sizeof(void *));
+  DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *));
+
+  lldb::offset_t offset;
+
+  uint64_t expected =
+      0b0111111100000001111111000000011111110000000111111100000001111111;
+  offset = 0;
+  EXPECT_EQ(expected, LE.GetULEB128(&offset));
+  EXPECT_EQ(9U, offset);
+  offset = 0;
+  EXPECT_EQ(expected, BE.GetULEB128(&offset));
+  EXPECT_EQ(9U, offset);
+}