[PATCH] D129728: [llvm-dwp] Add SHF_COMPRESSED support and remove .zdebug support

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: llvm/lib/DWP/DWP.cpp:282-285
+  bool IsLE = isa<object::ELF32LEObjectFile>(Obj) ||
+              isa<object::ELF64LEObjectFile>(Obj);
+  bool Is64 = isa<object::ELF64LEObjectFile>(Obj) ||
+              isa<object::ELF64BEObjectFile>(Obj);
----------------
dblaikie wrote:
> dblaikie wrote:
> > MaskRay wrote:
> > > dblaikie wrote:
> > > > This should probably be in a separate patch with test coverage (though generally I don't think adding functionality/doing work on llvm-dwp is really worth it - it probably needs to be thrown out & start again to make it sufficiently efficient in terms of memory usage, etc - though with the WebAssembly (so the costly abstractions are now load-bearing to support WebAssembly) support added & lld's non-generality (so doesn't have sufficient abstractions to make it trivial to implement over different formats), it's not clear what the foundation of such a new implementation would be, whether the generality can be maintained while achieving the desired performance, etc).
> > > > 
> > > > A FIXME could document what's entailed in adding the endian/size functionality here.
> > > Will add a TODO that this is untested (some binary utilities use TODO to indicate no test coverage as well).
> > > 
> > > Thanks for the insight that llvm-dwp probably should be re-implemented. I haven't followed the development...
> > > 
> > > (@ckissane)
> > I'd prefer it not be checked in untested. 
> Sorry, should've provided more context: There are some cases where I think untested functionality is warranted, when testing is particularly difficult & manual testing is done as a stop-gap.
> 
> In this case I don't think it's much of a value-add, the functionality isn't used much & sort of a paradox of "if it's used just enough to be useful/but then untested, that's especially unfortunate".
> 
> If there's concern about this misbehaving, I'd be open to an assertion with a FIXME about how to support other things if someone comes across the assertion.
It's actually not too difficult to test all 4 ELF types. Just added the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129728