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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 12 inline comments as done.
jankratochvil added a comment.

In D81119#2073973 <https://reviews.llvm.org/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.



================
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
----------------
labath wrote:
> 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.
I admit it works with `.o` now but when I was testing it before it did not (despite GDB could open such `.o` file).


================
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; }
----------------
labath wrote:
> At this point the file is far enough from the original, that this and the "patched" lines are not useful.
I disagree, this is a minimal C code to produce DWARF containing `DW_TAG_variable` + `DW_AT_const_value`. I have reworded the comment.


================
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
----------------
labath wrote:
> 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).
I just do not think it is worth the development time to make even testcases so optimal but OK.


================
Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:270
+                   sizeof(void *));
+  DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *));
+
----------------
labath wrote:
> 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).
I have kept them, what if someone makes there some endianity dependency, dunno.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81119