[Lldb-commits] [PATCH] D81119: [lldb] Fix SLEB128 decoding

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

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. :)



================
Comment at: lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s:1
+# This tests that lldb is able to process 64-bit wide negative SLEB128.
+
----------------
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.


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s:5
+
+# RUN: llvm-mc -g -dwarf-version=5 -triple x86_64-unknown-linux-gnu %s -filetype=obj > %t.o
+# RUN: ld.lld -m elf_x86_64 %t.o -o %t 
----------------
I don't think `-g -dwarf-version=5 ` is needed here.


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s:6
+# RUN: llvm-mc -g -dwarf-version=5 -triple x86_64-unknown-linux-gnu %s -filetype=obj > %t.o
+# RUN: ld.lld -m elf_x86_64 %t.o -o %t 
+# RUN: %lldb %t -o "p/x magic64" -o exit | FileCheck %s
----------------
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.


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s:11-13
+# It is built from this source with a few #-marked patched lines:
+# static const long magic64 = 0xed9a924c00011151;
+# int main(void) { return magic64; }
----------------
At this point the file is far enough from the original, that this and the "patched" lines are not useful.


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s:24-28
+	.asciz	"clang version 10.0.0" # string offset=0
+.Linfo_string3:
+	.asciz	"magic64"               # string offset=74
+.Linfo_string4:
+	.asciz	"long int"              # string offset=82
----------------
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).


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/dwarf-sleb128.s:84
+#	.ascii	"\321\242\204\200\300\311\244\315\355\001" # DW_AT_const_value
+	.ascii	"\321\242\204\200\300\311\244\315m" # DW_AT_const_value - sdata
+.Lconst:
----------------
`.sleb128 0xed9a924c00011151`


================
Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:270
+                   sizeof(void *));
+  DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *));
+
----------------
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).


================
Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:292
+
+  int64_t expected = 0b0111111100000001111111000000011111110000000111111100000001111111;
+  offset = 0;
----------------
Should this be uint64_t ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81119