[Lldb-commits] [PATCH] D66791: [lldb][ELF] Read symbols from .gnu_debugdata sect.

Fri Sep 6 02:49:52 PDT 2019

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