[PATCH] D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections.

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

In https://reviews.llvm.org/D49678#1176327, @jhenderson wrote:

> In https://reviews.llvm.org/D49678#1176190, @jakehehrlich wrote:
>
> > Also I think you can build llvm with or without zlib. I think that means you're going to need to #ifdef a lot of this code.
>
>
> As an additional point, it would be good to minimise the number of distinct areas where we have to add these #ifdefs. I'd recommend trying to hide as much as possible of this behind an interface of some kind, which turns into a no-op or warning/error if --compress-debug-sections is used without zlib support.


Also I wanted to say I'm pro this. This should be achieved regardless of which idea we settle on.

In https://reviews.llvm.org/D49678#1176841, @plotfi wrote:

> In https://reviews.llvm.org/D49678#1176191, @jakehehrlich wrote:
>
> > Also also, why do you need this? I ask because from talking with some people about what this is used for there is almost certainly a better way to accomplish any gain you get out of it. The main reason I can still see to do this is to achieve command line compatibility; if that is the goal we can probably noop or just noop with a section rename.
> >
> > I don't appose actually implementing this BTW, I'm just wondering if there isn't something both simplair and better you could be doing.
>
>
> I don't understand. Do you mean the entire patch or a specific part of it? The purpose is to have feature parity with GNU Objcopy in this respect.


Right, I don't personally care about reaching parity with GNU objcopy. The goal as I see it is to make sure it's usable as a drop in replacement everywhere. That doesn't mean that we need to actually implement the same behavior for any given flag, nor does it mean that we need to implement all flags. In general I've only added flags when they either had a direct need in some known project or were very simple to implement. I see these compression flags as not being useful anymore because better options exist in almost all cases. llvm-objcopy should still implement the flags to support using llvm-objcopy as a drop in replacement. It's strictly better if we actually compress the sections properly but it's far from the end of the world if we just rename the sections and do nothing else. That would achieve the main goal with minimal work. I for instance did this with --only-keep-debug. The savings in implementing that flag properly are minimal but being able to not have llvm-objcopy barf on that flag has huge benefits. I see these flags as being similar.



================
Comment at: tools/llvm-objcopy/Object.h:215
 
+  constexpr bool isBigEndian() const {
+    static_assert((std::is_same<ELFT, object::ELF64LE>::value ||
----------------
plotfi wrote:
> jakehehrlich wrote:
> > You should probably not be checking this ever.  I'm going to need a very *very* good reason to allow this function to be used. The whole reason we (and LLD) haven't had any bugs with endianess is because we don't do these sorts of checks. Instead we use the integer types from ELFT in a uniform way.
> I can remove the static_asserts. Is that what you meant? Or are you saying don't pass isBigEndian to the compressor? Could you clarify? 
No I mean you shouldn't be converting these to bools at all. We currently only do this kind of thing at one point in the code and I'd prefer we not do it at all. In this case there is a clear way to avoid it by just using ELFT::Chdr as it was intended to be used.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678