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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to