[PATCH] D60189: [llvm-objcopy] Simplify SHT_NOBITS -> SHT_PROGBITS promotion

Tue Apr 9 11:32:47 PDT 2019

rupprecht added a comment.

In D60189#1459556 <https://reviews.llvm.org/D60189#1459556>, @jakehehrlich wrote:

> I haven't bread or considered this patch. I just want to clarify when I think we should make changes of this type.
>
> We should do things "the right way" at first and then comply more closely to GNU objcopy as real world cases come up. We should make minimal comparability attempts. Effectively think of it as "premature comparability isn't he root of all evil". We can optimize for compatibility when the case comes up.
>
> As for working backwards from where we may have over optimized in the past and doing something more sensible now, that is kind of harder. To make such a change we need to know that we're not going to break anyone. Without evidence for that we have to reject such changes.
>
> If Jordan can build all of Google's code with such a change then that is strong evidence that this is ok. Any other large or complicated code base would also be valid I think. These kinds of checks are very expensive however some should only request such evidence if the use case is very compelling.

FWIW, my original change to promote from NOBITS to PROGBITS for this flag wasn't caught by any internal code -- I think we are only using these flags in situations where the section is already PROGBITS anyway. It was caught by running the gnu binutils test suite against llvm-objcopy. My latest round (which has been a while) didn't show anything related to this area.

This changes seems pretty reasonable to me, but this gets into the ugly area of how close to GNU compatibility do we need to get. If someone is relying on GNU objcopy behavior to promote a NOBITS section to PROGBITS section only by setting the `noload` flag, then this change will break them, but that also seems like an extremely obscure/broken use of this tool that we shouldn't support. But I haven't done much investigation, hence I'm hoping someone else has an opinion :) if not I can take a more thorough look later

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60189