[PATCH] D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D49870#1182950, @rupprecht wrote:

> In https://reviews.llvm.org/D49870#1181835, @jhenderson wrote:
>
> >
>
>
> I don't really have an immediate preference, so I just added those two to the preserve mask -- let me know if I should add others.


I think this is a reasonable approach for now. I can't think of any situation where converting between TLS and non-TLS (and vice versa) makes sense, since the relocations needed are different.

> Yep, I was thinking something like `objcopy --rename-section .foo=.bar,shf_alloc,shf_execinstr,noshf_write...` might be a good way to build that into this.

I'd probably drop the "shf" (alloc is equivalent to SHF_ALLOC as far as I'm aware), and I'd make them all explicitly set (so no "noshf_*" flags), to avoid weird behaviour.

> BTW, the use case I'm currently trying to fix is `objcopy -I binary -Bi386:x86-64 -Oelf64-x86-64 --rename-section .data=.rodata,alloc,load,readonly,data,contents`, mentioned here: http://lists.llvm.org/pipermail/llvm-dev/2017-June/113655.html. It would make my life (and I think many others' lives) a lot easier if we could at least support those options for drop-in compatibility, but agree that we don't need to go full-on bug for bug compatibility.

Sure. I think the only thing I'd change is to remove mentioning the GNU objcopy supported flags from a) the error message and b) the help text, until we agree on the "ideal" interface. We should still support those flags, but if we make them hidden, it means people will (hopefully) be more likely to use any new interface we propose going forward.

Okay, maybe here's my best, hopefully uncontroversial, suggestion, since it would be good to get this in for the LLVM 7.0 cut today(?): leave them in but mark them as "legacy" or for GNU compatibility e.g: `Flags supported for GNU compatibility: alloc, load...` in both error message and help text. If and when we add more flags, for a better/clearer UI, we can make that a separate sentence like `Supported Flags: alloc, write, execinstr,... Additional flags supported for GNU compatibility: load, noload...` etc.

LGTM with those changes.



================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:32-35
+                      HelpText<"Renames a section from old to new, optionally "
+		               "with specified flags. Supported flags: alloc, "
+			       "load, noload, readonly, debug, code, data, "
+			       "rom, share, contents, merge, strings.">;
----------------
jhenderson wrote:
> Is this clang-formatted? It looks weird...
I just realised that this is in the .td file, so clang-format might not be applicable, but it looks better now anyway, thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D49870