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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The code looks correct to me (barring a couple of nits) for what you are trying to do, but I do have a couple of outstanding concerns, that I'd like to discuss further:

I'm really worried about clearing the TLS flag, and to a lesser extent, the INFO_LINK flag. Clearing the TLS flag would almost certainly break the file, and I have no idea what the repercussions of clearing INFO_LINK might be. I'd be tempted to break with GNU compatibility here and not clear either of them. If we really don't want to do that, I'd recommend including a "tls" input option. I'm not sure what to do with INFO_LINK, because it affects other fields in the section header. Are you sure that the INFO_LINK and TLS flags are being cleared by the renaming and not by something else (e.g. implicit meaning of section names)? What happens if you rename e.g. .tdata.foo to .tdata.bar (thus maintaining the standard TLS naming scheme)?

I've been thinking about this a bit more, and I honestly feel like it doesn't make sense to publish the somewhat arcane options that GNU objcopy supports, except for legacy support. As such, whilst I believe we should support things like "contents", "debug" etc, in order to maintain compatibility, I also feel like they shouldn't be in the help text, to try to "persuade" people to adopt a more sensible and meaningful usage going forwards. Going on from this, how would you feel about extending the syntax to make more sense for ELF, i.e. to have a flag name for each modifiable individual ELF flag (i.e. alloc, write, execinstr, tls, ...)? I don't think this needs to happen in this change, but I do feel like we shouldn't be satisfied with just GNU compatibility.



================
Comment at: test/tools/llvm-objcopy/rename-section-flag-preserved.test:1
+# RUN: yaml2obj %s > %t
+
----------------
Since this uses an OS-specific flag, will this work without x86_64 target support? Or do we need a REQUIRES?


================
Comment at: test/tools/llvm-objcopy/rename-section-flag-preserved.test:3
+
+# Single flags on a section w/ all flags:
+# RUN: llvm-objcopy --rename-section=.foo=.bar,alloc %t %t.alloc
----------------
Nit: no need to abbreviate this. Just use "with" explicitly.


================
Comment at: test/tools/llvm-objcopy/rename-section-flag-preserved.test:52
+                       SHF_OS_NONCONFORMING, SHF_STRINGS, SHF_TLS,
+		       SHF_X86_64_LARGE, SHF_WRITE ]
+    Content:        "a4a4a4a4"
----------------
Nit: You should indent this to the same level as the previous line.


================
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.">;
----------------
Is this clang-formatted? It looks weird...


Repository:
  rL LLVM

https://reviews.llvm.org/D49870