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

Fri Sep 6 05:05:43 PDT 2019

kwk added a comment.

I've tackled the easy comments now and will come back to the more detailed ones as well.  I'm not ignoring them.

================
Comment at: lldb/CMakeLists.txt:101

-  set(LLDB_TEST_DEPS lldb)
+  set(LLDB_TEST_DEPS lldb llvm-nm llvm-strip)

----------------
labath wrote:
> 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?
You're right. one place is enough. 

================
Comment at: lldb/include/lldb/Host/LZMA.h:24-25
+
+llvm::Error getUncompressedSize(llvm::ArrayRef<uint8_t> InputBuffer,
+                                uint64_t &uncompressedSize);
+
----------------
labath wrote:
> 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?
I've changed the syntax as you suggested. I kept the function there because I find it easier to maintain this way.

================
Comment at: lldb/lit/Breakpoint/minidebuginfo-corrupt-xz.yaml:1
+# REQUIRES: system-linux, lzma
+
----------------
labath wrote:
> 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.
I've move all to `lit/Modules/ELF`.

================
Comment at: lldb/lit/Breakpoint/minidebuginfo-corrupt-xz.yaml:13
+
+# RUN: %lldb -x -b -o 'image dump symtab' %t.obj 2>&1 | FileCheck %s
+
----------------
labath wrote:
> You shouldn't need an explicit `-x` in these tests as %lldb adds `--no-use-colors` already.
I use `-x` and not `-X`. See `lldb --help`:

`-x                   Alias for --no-lldbinit`
`-X                    Alias for --no-use-color`

================
Comment at: lldb/lit/Breakpoint/minidebuginfo-no-lzma.yaml:22
+    AddressAlign:    0x0000000000000001
+    Content:         FD377A585A000004E6D6B4460200210116000000742FE5A3E0180F05BA5D003F914584683D89A6DA8ACC93E24ED90802EC1FE2A7102958F4A42B6A7134F23922F6F35F529E133A8B5588025CFAC876C68510A157DBBCF8CA75E9854DED10FDD5CE0CDC136F6459B13B9847AEF79E9B1C7CD70EF4F3AF709F5DA0C1F40780154D72120A6A62A3F1A216E20DC597CE55BB23B48785957321A15FEE48808C1428B925DBC8022541CC594BD0AF2B51C6BE2854C81611017704DF6E509D21013B80BEC27D8919ACD3157E89353A08F4C86781ED708E89AB322D010F0F1605DAD9B9CE2B13C387769C83F5F85C647FD9C551E0E9C7D4A5CBE297970E486CB94AC283F98A7C6412A57F9C37952327549EEC4634D2CFA55B0F99923A14992D4293E0D87CEEF7FB6160C45928DE25074EEBF5329B5579AF01DB23DF22CBD48C8037B68FFFBE5CEA6CD26A936DD07D9B2E6006B7C6E5CC751072185EFE995D3F3C8DACF9039D4BEFB1F376B491568F6F00DB50FF477F36B90413E4FA30AE7C561A1249FD45FDFF884F70247FC21E57195A764151D8E341267E724D856C512BD243CDB33AB313758443877B2CB58F7F8F0461DE9766647F333A3531BDC4A26E9537EB314708D31212FCF4C21E9CB139F4DBFD21BB16A126C35E2BB3F7E30BF5A54961CECD4DD4D91A3757356F618754B21533C34F2BD97D70A02B1F338588BDBA9CDF5FC9FBE973E550194F07EC7A1E8E3C005FD60F8853223427628987E82E701CA7E2FDFA1B0ED564C37D115A72C3EC01E29C85C3630D8A385C4AE12F4F75F9F0BC12F2698345DD62A1F546A5953AF5CF3C0F22C7DA510F6739EB8CDB0E8A5A3BC13CFC31C1875C313908EFF23678869B76A6E1C10FE699E43BFFDE8F0752ED994A4A84BC0AD9D7381131D457C4917C4F6656F5C95D3221A79166C802D5F5A7C68554E54C42CA535465D224C7B641CF3417C3EAFD03CE5709BEA33DC7C9155CAC9D3C8033AF7CDA622020606A7C139D77FF85BC19323BF956C9C4662F60079BC7FE5F67B46211716A1A6CE4AB8AAB307D6444310CBC101071703EECC0B4622D91D705F5DA2932DA8BCEDA8E1CB0CDB20AAD652B8F86A521D3421287F1C175AE3BE6458AE6F8F3FB6FB7ED97B616B580D791E5FE0B74973F8604F419039B5B9D9A14397EE509F2B33AE404FF96DD0551472C5302E67910F0794B15CFE837351C6AF89B2FE88488B557BE8ACFFA331FB7AD553D35CAEB7D8BCEFB6CFF4A58E91355FE931408CF4CAFA9B97518B9E5C02078F64CE81279801B090348213DCAA7D12DC098BFF58C5A3202EFC38F64AD894379747B54AB5A9843F82E5FF1F394C8B783C3A8F1655DDEF8D5FE09EBB3E703853ABD716743507000696FB6B35216B088E499F53880375521442ED45DCDD1B31AAEBDAD3C7DA958593425206C4B2A0BC6CADE3B0B1598499E08016E84F33E3EB9D7B03B9C9DFA91B8CE5C74DEF2BC97FEE9982B0AEC16C75EEB7AE9A858A9C37F6C12B040C68A49111DCF0F3A4780F3879E93D904676BE908FDC66373D34AA715A39EFBC2795C6C8F058CA24392FB2591AD06ACD6AED8746F926886180C2B007ED58C9884A8BEF6CCA1F549F5C4FB411A3FF78770D1147363AC80B98B5A8FDB3DEC4E61709F66A622BDA835B1FD67B7C7CB166ABB163FB7C5204AE200C71C6A18B532C407647323B8F2FAC7ECB68C250877FC8DD5FE05B2B39E66F687EBB6EEFB7D5209E22F451B76F57D90BB6641DFFDE1A1821C4D783E4756F3CEE7F63B9BA284F8E114B0D9A086D83233BED4A8F5B60933DC16AF4DDE19C9FC59BCC1646343ECE7007B1C4DC65C4A939CDD47F6ED8855913183149BECE66D8FE7793AE607EB8E28513749B9548252764110D3B58D1D8B348DB18F7F24F8CA0C7D9CB515D90F7F1848FF58472B2EF52EBAB123AFC7F87890CE9FC55B31160014294A9B7F81638A27335E29E15A10B1068D5E049B1C239814DBBCC1BB30E11EEBAD5ACF8FB1B986C4F48D73FEA6129D9708A0B5AC435402BEC8C79C71DB94394811B9A604141A125A4669F9A139A0264E93E822117BE8E0D93A1487C51214E9FBF5763A3FBE9DA700B9C9B435472AF9F0B4446B000000003239307DD8B645100001D60B90300000CA1EC9E9B1C467FB020000000004595A
+...
----------------
labath wrote:
> 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...
I kept it the same for the like the other files so you can text-diff it to find out that only one byte was modified. I thought this is a neat idea in order to actually error out upon decompression and not on file header for example. That being said what do you think about adding tests for the different errors that can occur during the whole inspection (e.g. in getUncompressedSize). 

================
Comment at: lldb/source/Host/common/LZMA.cpp:81-82
+        llvm::inconvertibleErrorCode(),
+        "size of xz-compressed blob ({0} bytes) is smaller than the "
+        "LZMA_STREAM_HEADER_SIZE ({1} bytes)",
+        compressedBufferSize, LZMA_STREAM_HEADER_SIZE);
----------------
labath wrote:
> printf format string here.
All done now.

================
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:
> labath wrote:
> > You don't need the temporary ArrayRef. You can just pass `data.GetData()` directly..
> What about this?
Sorry for missing this. Done now.

================
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()) {
----------------
labath wrote:
> 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.
Done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791