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

Fri Sep 6 05:21:04 PDT 2019

labath marked an inline comment as done.
labath added inline comments.

================
Comment at: lldb/include/lldb/Host/LZMA.h:24-25
+
+llvm::Error getUncompressedSize(llvm::ArrayRef<uint8_t> InputBuffer,
+                                uint64_t &uncompressedSize);
+
----------------
kwk wrote:
> 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.
Ok. I am fine with that.

================
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:
> kwk wrote:
> > 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`
> Actually, I take this back. It seems %lldb does not pass this automatically. I am not sure why, but since colors aren't a problem you here, you probably don't have to explicitly disable them anyway...
Ah, my bad. In that case you definitely *can* remove it because %lldb does add `--no-lldbinit`.

================
Comment at: lldb/lit/Breakpoint/minidebuginfo-no-lzma.yaml:22
+    AddressAlign:    0x0000000000000001
+    Content:         FD377A585A000004E6D6B4460200210116000000742FE5A3E0180F05BA5D003F914584683D89A6DA8ACC93E24ED90802EC1FE2A7102958F4A42B6A7134F23922F6F35F529E133A8B5588025CFAC876C68510A157DBBCF8CA75E9854DED10FDD5CE0CDC136F6459B13B9847AEF79E9B1C7CD70EF4F3AF709F5DA0C1F40780154D72120A6A62A3F1A216E20DC597CE55BB23B48785957321A15FEE48808C1428B925DBC8022541CC594BD0AF2B51C6BE2854C81611017704DF6E509D21013B80BEC27D8919ACD3157E89353A08F4C86781ED708E89AB322D010F0F1605DAD9B9CE2B13C387769C83F5F85C647FD9C551E0E9C7D4A5CBE297970E486CB94AC283F98A7C6412A57F9C37952327549EEC4634D2CFA55B0F99923A14992D4293E0D87CEEF7FB6160C45928DE25074EEBF5329B5579AF01DB23DF22CBD48C8037B68FFFBE5CEA6CD26A936DD07D9B2E6006B7C6E5CC751072185EFE995D3F3C8DACF9039D4BEFB1F376B491568F6F00DB50FF477F36B90413E4FA30AE7C561A1249FD45FDFF884F70247FC21E57195A764151D8E341267E724D856C512BD243CDB33AB313758443877B2CB58F7F8F0461DE9766647F333A3531BDC4A26E9537EB314708D31212FCF4C21E9CB139F4DBFD21BB16A126C35E2BB3F7E30BF5A54961CECD4DD4D91A3757356F618754B21533C34F2BD97D70A02B1F338588BDBA9CDF5FC9FBE973E550194F07EC7A1E8E3C005FD60F8853223427628987E82E701CA7E2FDFA1B0ED564C37D115A72C3EC01E29C85C3630D8A385C4AE12F4F75F9F0BC12F2698345DD62A1F546A5953AF5CF3C0F22C7DA510F6739EB8CDB0E8A5A3BC13CFC31C1875C313908EFF23678869B76A6E1C10FE699E43BFFDE8F0752ED994A4A84BC0AD9D7381131D457C4917C4F6656F5C95D3221A79166C802D5F5A7C68554E54C42CA535465D224C7B641CF3417C3EAFD03CE5709BEA33DC7C9155CAC9D3C8033AF7CDA622020606A7C139D77FF85BC19323BF956C9C4662F60079BC7FE5F67B46211716A1A6CE4AB8AAB307D6444310CBC101071703EECC0B4622D91D705F5DA2932DA8BCEDA8E1CB0CDB20AAD652B8F86A521D3421287F1C175AE3BE6458AE6F8F3FB6FB7ED97B616B580D791E5FE0B74973F8604F419039B5B9D9A14397EE509F2B33AE404FF96DD0551472C5302E67910F0794B15CFE837351C6AF89B2FE88488B557BE8ACFFA331FB7AD553D35CAEB7D8BCEFB6CFF4A58E91355FE931408CF4CAFA9B97518B9E5C02078F64CE81279801B090348213DCAA7D12DC098BFF58C5A3202EFC38F64AD894379747B54AB5A9843F82E5FF1F394C8B783C3A8F1655DDEF8D5FE09EBB3E703853ABD716743507000696FB6B35216B088E499F53880375521442ED45DCDD1B31AAEBDAD3C7DA958593425206C4B2A0BC6CADE3B0B1598499E08016E84F33E3EB9D7B03B9C9DFA91B8CE5C74DEF2BC97FEE9982B0AEC16C75EEB7AE9A858A9C37F6C12B040C68A49111DCF0F3A4780F3879E93D904676BE908FDC66373D34AA715A39EFBC2795C6C8F058CA24392FB2591AD06ACD6AED8746F926886180C2B007ED58C9884A8BEF6CCA1F549F5C4FB411A3FF78770D1147363AC80B98B5A8FDB3DEC4E61709F66A622BDA835B1FD67B7C7CB166ABB163FB7C5204AE200C71C6A18B532C407647323B8F2FAC7ECB68C250877FC8DD5FE05B2B39E66F687EBB6EEFB7D5209E22F451B76F57D90BB6641DFFDE1A1821C4D783E4756F3CEE7F63B9BA284F8E114B0D9A086D83233BED4A8F5B60933DC16AF4DDE19C9FC59BCC1646343ECE7007B1C4DC65C4A939CDD47F6ED8855913183149BECE66D8FE7793AE607EB8E28513749B9548252764110D3B58D1D8B348DB18F7F24F8CA0C7D9CB515D90F7F1848FF58472B2EF52EBAB123AFC7F87890CE9FC55B31160014294A9B7F81638A27335E29E15A10B1068D5E049B1C239814DBBCC1BB30E11EEBAD5ACF8FB1B986C4F48D73FEA6129D9708A0B5AC435402BEC8C79C71DB94394811B9A604141A125A4669F9A139A0264E93E822117BE8E0D93A1487C51214E9FBF5763A3FBE9DA700B9C9B435472AF9F0B4446B000000003239307DD8B645100001D60B90300000CA1EC9E9B1C467FB020000000004595A
+...
----------------
kwk wrote:
> 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). 
Ok. That's fine with me.
I am definitely not *against* adding more tests. :) I am not going to ask you to put more in because this patch already has more tests than most lldb patches, but I you want to do that, then by all means, go ahead. Things like these would be probably better off as a (c++) unit test, as they're really testing the details of the lzma handling code. The elf code doesn't really care about these details -- it just wants to know whether the decompression was successful or not.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66791