labath added a comment.
Thank you for adding the tests, and for adding the `xz` detection to lit in
particular. We're getting _reaally_ close now. I have a bunch more comments,
including highlighting some you seems to have missed from earlier, but they are
all just about polishing. The only somewhat larger comment is about the
`minidebuginfo-set-and-hit-breakpoint.test` test. I'd like to understand what
exactly is the scenario you are intending to test there, because I have a
feeling it's not testing what you want it to test...
================
Comment at: lldb/CMakeLists.txt:101
- set(LLDB_TEST_DEPS lldb)
+ set(LLDB_TEST_DEPS lldb llvm-nm llvm-strip)
----------------
labath wrote:
> It looks like you're already adding these in `lldb/lit/CMakeLists.txt`. Does
> it need to be in both?
What about this?
================
Comment at: lldb/include/lldb/Host/LZMA.h:24-25
+
+llvm::Error getUncompressedSize(llvm::ArrayRef<uint8_t> InputBuffer,
+ uint64_t &uncompressedSize);
+
----------------
labath wrote:
> Now that the vector is auto-resized, do you think it's still useful to expose
> the `getUncompressedSize` function as public api? If yes, then please change
> it to return Expected<uint64_t>. (returning Expected would be nice even for a
> private api, but there it's less important...)
What about this?
================
Comment at: lldb/lit/Breakpoint/minidebuginfo-corrupt-xz.yaml:1
+# REQUIRES: system-linux, lzma
+
----------------
These tests should go to `lit/Modules/ELF`, as they have nothing to do with
breakpoints, and everything to do with ELF files. The
`minidebuginfo-set-and-hit-breakpoint.test` might stay here as it actually does
deal with breakpoints, but it would also make sense to put it next to the ELF
tests. I'll leave it up to you to decide where to place that one.
================
Comment at: lldb/lit/Breakpoint/minidebuginfo-corrupt-xz.yaml:13
+
+# RUN: %lldb -x -b -o 'image dump symtab' %t.obj 2>&1 | FileCheck %s
+
----------------
You shouldn't need an explicit `-x` in these tests as %lldb adds
`--no-use-colors` already.
================
Comment at: lldb/lit/Breakpoint/minidebuginfo-find-symbols.yaml:15
+ Entry: 0x00000000004004C0
+Sections:
+ - Name: .gnu_debugdata
----------------
Could you add a symbol or two to the dynsym section of the outer file? I'd like
to test that we get symbols from .dynsym *and* .symtab sections.
================
Comment at: lldb/lit/Breakpoint/minidebuginfo-no-lzma.yaml:22
+ AddressAlign: 0x0000000000000001
+ Content:
FD377A585A000004E6D6B4460200210116000000742FE5A3E0180F05BA5D003F914584683D89A6DA8ACC93E24ED90802EC1FE2A7102958F4A42B6A7134F23922F6F35F529E133A8B5588025CFAC876C68510A157DBBCF8CA75E9854DED10FDD5CE0CDC136F6459B13B9847AEF79E9B1C7CD70EF4F3AF709F5DA0C1F40780154D72120A6A62A3F1A216E20DC597CE55BB23B48785957321A15FEE48808C1428B925DBC8022541CC594BD0AF2B51C6BE2854C81611017704DF6E509D21013B80BEC27D8919ACD3157E89353A08F4C86781ED708E89AB322D010F0F1605DAD9B9CE2B13C387769C83F5F85C647FD9C551E0E9C7D4A5CBE297970E486CB94AC283F98A7C6412A57F9C37952327549EEC4634D2CFA55B0F99923A14992D4293E0D87CEEF7FB6160C45928DE25074EEBF5329B5579AF01DB23DF22CBD48C8037B68FFFBE5CEA6CD26A936DD07D9B2E6006B7C6E5CC751072185EFE995D3F3C8DACF9039D4BEFB1F376B491568F6F00DB50FF477F36B90413E4FA30AE7C561A1249FD45FDFF884F70247FC21E57195A764151D8E341267E724D856C512BD243CDB33AB313758443877B2CB58F7F8F0461DE9766647F333A3531BDC4A26E9537EB314708D31212FCF4C21E9CB139F4DBFD21BB16A126C35E2BB3F7E30BF5A54961CECD4DD4D91A3757356F618754B21533C34F2BD97D70A02B1F338588BDBA9CDF5FC9FBE973E550194F07EC7A1E8E3C005FD60F8853223427628987E82E701CA7E2FDFA1B0ED564C37D115A72C3EC01E29C85C3630D8A385C4AE12F4F75F9F0BC12F2698345DD62A1F546A5953AF5CF3C0F22C7DA510F6739EB8CDB0E8A5A3BC13CFC31C1875C313908EFF23678869B76A6E1C10FE699E43BFFDE8F0752ED994A4A84BC0AD9D7381131D457C4917C4F6656F5C95D3221A79166C802D5F5A7C68554E54C42CA535465D224C7B641CF3417C3EAFD03CE5709BEA33DC7C9155CAC9D3C8033AF7CDA622020606A7C139D77FF85BC19323BF956C9C4662F60079BC7FE5F67B46211716A1A6CE4AB8AAB307D6444310CBC101071703EECC0B4622D91D705F5DA2932DA8BCEDA8E1CB0CDB20AAD652B8F86A521D3421287F1C175AE3BE6458AE6F8F3FB6FB7ED97B616B580D791E5FE0B74973F8604F419039B5B9D9A14397EE509F2B33AE404FF96DD0551472C5302E67910F0794B15CFE837351C6AF89B2FE88488B557BE8ACFFA331FB7AD553D35CAEB7D8BCEFB6CFF4A58E91355FE931408CF4CAFA9B97518B9E5C02078F64CE81279801B090348213DCAA7D12DC098BFF58C5A3202EFC38F64AD894379747B54AB5A9843F82E5FF1F394C8B783C3A8F1655DDEF8D5FE09EBB3E703853ABD716743507000696FB6B35216B088E499F53880375521442ED45DCDD1B31AAEBDAD3C7DA958593425206C4B2A0BC6CADE3B0B1598499E08016E84F33E3EB9D7B03B9C9DFA91B8CE5C74DEF2BC97FEE9982B0AEC16C75EEB7AE9A858A9C37F6C12B040C68A49111DCF0F3A4780F3879E93D904676BE908FDC66373D34AA715A39EFBC2795C6C8F058CA24392FB2591AD06ACD6AED8746F926886180C2B007ED58C9884A8BEF6CCA1F549F5C4FB411A3FF78770D1147363AC80B98B5A8FDB3DEC4E61709F66A622BDA835B1FD67B7C7CB166ABB163FB7C5204AE200C71C6A18B532C407647323B8F2FAC7ECB68C250877FC8DD5FE05B2B39E66F687EBB6EEFB7D5209E22F451B76F57D90BB6641DFFDE1A1821C4D783E4756F3CEE7F63B9BA284F8E114B0D9A086D83233BED4A8F5B60933DC16AF4DDE19C9FC59BCC1646343ECE7007B1C4DC65C4A939CDD47F6ED8855913183149BECE66D8FE7793AE607EB8E28513749B9548252764110D3B58D1D8B348DB18F7F24F8CA0C7D9CB515D90F7F1848FF58472B2EF52EBAB123AFC7F87890CE9FC55B31160014294A9B7F81638A27335E29E15A10B1068D5E049B1C239814DBBCC1BB30E11EEBAD5ACF8FB1B986C4F48D73FEA6129D9708A0B5AC435402BEC8C79C71DB94394811B9A604141A125A4669F9A139A0264E93E822117BE8E0D93A1487C51214E9FBF5763A3FBE9DA700B9C9B435472AF9F0B4446B000000003239307DD8B645100001D60B90300000CA1EC9E9B1C467FB020000000004595A
+...
----------------
Nit: You most likely don't need such a gigantic blob to test this bit of
functionality, as you can't read the data anyway...
================
Comment at: lldb/lit/Breakpoint/minidebuginfo-set-and-hit-breakpoint.test:50
+
+# RUN: LD_LIBRARY_PATH=$LD_LIBRARY_PATH:%T %lldb -x -b -o 'b multiplyByThree'
-o 'b multiplyByFour' -o 'breakpoint list -v' -o 'run' -o 'continue' %t.binary
| FileCheck %s
+
----------------
Instead of messing with the environment, it might be better to just set the
rpath (`-Wl,-rpath`) of the executable appropriately (but I would still like to
know what you're exactly trying to test with that extra shared library).
================
Comment at: lldb/lit/lit.cfg.py:102
+
+if config.lldb_enable_lzma == "ON":
+ config.available_features.add('lzma')
----------------
labath wrote:
> you should apply `llvm_canonicalize_cmake_booleans` to the value of
> LLDB_ENABLE_LZMA at the cmake level. Otherwise, this will blow up if someone
> types `-DLLDB_ENABLE_LZMA=On`
What about this?
================
Comment at: lldb/source/Host/common/LZMA.cpp:72-82
+ auto opts = lzma_stream_flags{};
+ if (InputBuffer.size() < LZMA_STREAM_HEADER_SIZE) {
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "size of xz-compressed blob (%lu bytes) is smaller than the "
+ "LZMA_STREAM_HEADER_SIZE (%lu bytes)",
+ InputBuffer.size(), LZMA_STREAM_HEADER_SIZE);
----------------
labath wrote:
> too much auto
What about this? (I think both of the autos here are unnecessary).
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1850
+ // If there's none in the orignal object file, we add it to it.
+ SectionList *module_section_list = GetModule()->GetSectionList();
+ if (auto gdd_obj_file = GetGnuDebugDataObjectFile()) {
----------------
I didn't notice this before, but you shouln't be calling
module->GetSectionList() here. The `unified_section_list` you got as an
argument *is* the module's section list.
================
Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1886
+ section->GetSectionData(data);
+ llvm::ArrayRef<uint8_t> compressedData(data.GetDataStart(),
data.GetByteSize());
+ llvm::SmallVector<uint8_t, 0> uncompressedData;
----------------
labath wrote:
> You don't need the temporary ArrayRef. You can just pass `data.GetData()`
> directly..
What about this?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66791/new/
https://reviews.llvm.org/D66791
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
