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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
