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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

My main concern is still the interaction between these switches and existing flags. Specifically, I'm worried about what will happen if this is used on a section with lots of flags that cannot be controlled via this switch. A quick look at the spec shows that this will clear the following flags unconditionally: SHF_INFO_LINK, SHF_GROUP, SHF_TLS, SHF_COMPRESSED, and any OS/processor-specific flags. I haven't checked what GNU objcopy does, but in basically all of those, changing the flags could be bad, and have a negative impact on the ability to link the resultant object. Could you investigate what GNU does in each of these cases, and write tests to show the behaviour, please.

Also, I feel like we should have two versions of the basic test: one with a section with no flags, and the other with a section with all the flags, if possible, to show which get set, which get unset, and which get ignored. Perhaps this would be best achieved with two sections, one each of the two different flag sets outlined in the previous sentence, and the main test just runs objcopy/readobj/FileCheck twice or similar.



================
Comment at: test/tools/llvm-objcopy/rename-section-flag.test:49
+# LOAD-NEXT: Flags [ (0x1)
+# LOAD-NEXT: SHF_WRITE (0x1)
+# LOAD-NEXT: ]
----------------
jakehehrlich wrote:
> rupprecht wrote:
> > jhenderson wrote:
> > > I'm a bit surprised by this one. I haven't checked, but I'd expect load to cause SHF_ALLOC.
> > That's what I thought as well. However,
> > ```
> > $ readelf -S /tmp/a.o | grep -A 1 .text
> >   [ 2] .text             PROGBITS         0000000000000000  00000040
> >        0000000000000000  0000000000000000  AX       0     0     4
> > $ objcopy --rename-section=.text=.foo,load /tmp/a.o /tmp/a-gnu.o; readelf -S /tmp/a-gnu.o | grep -A 1 .foo
> >   [ 1] .foo              PROGBITS         0000000000000000  00000040
> >        0000000000000000  0000000000000000   W       0     0     4
> > 
> > ```
> Ah ok, so if you add any flag at all *then* this is correct. if however you add no flag it does not change the flags. As much as I dislike this behavoir it's critical we match GNU objcopy here...sigh...
Okay. I dug into GNU objcopy a little bit more, and that sort of makes sense, since these flags in theory affect more than the section flags, and look to be intended to be cross-platform.


================
Comment at: test/tools/llvm-objcopy/rename-section-flag.test:15
+# RUN: llvm-objcopy --rename-section=.foo=.bar,code %t %t.code
+# RUN: llvm-readobj -sections %t.code | FileCheck %s --check-prefixes=CHECK,CODE,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,data %t %t.data
----------------
I think this should be "EXEC" (or possibly even "EXECINSTR") not "CODE", just to make it completely clear what we mean.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:247
+    SectionFlag Flags = SectionFlag::SecNone;
+    for (size_t i = 1; i < NameAndFlags.size(); ++i) {
+      SectionFlag Flag = ParseSectionRenameFlag(NameAndFlags[i]);
----------------
i -> I
In addition, LLVM style is to put the termination size value in the initializer portion of for loops like so:
`for (size_t I = 1, Size = NameAndFlags.size(); I < Size; ++I)`


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:251
+      if (Flag == SectionFlag::SecNone)
+        error("unrecognized section flag '" + NameAndFlags[i] + "'");
+      Flags |= Flag;
----------------
Since, as I mentioned, exact identity on error messages is unimportant, in my opinion, why not just make this a long message with all the supported values listed either on the same line, or with an explicit new line?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:233
+  if (!FlagValue.contains('='))
+    error("Bad format for --rename-section: expected '='");
+
----------------
rupprecht wrote:
> alexshap wrote:
> > is there a test for this error ? if not - would you mind adding one ?
> > 
> > + question (just in case): do the error messages match binutils' ones ?
> There are tests for these -- and actually this once deviating from binutils, so I'll revert it.
> I've updated the other error messages to match binutils, with the exception of unrecognized section flag, which requires a minor change to the error() method.
> 
> ```
> $ objcopy --rename-section=.foo=.bar,xyz 
> objcopy: unrecognized section flag `xyz'
> objcopy: supported flags: alloc, load, noload, readonly, debug, code, data, rom, share, contents, merge, strings
> ```
> (the error methods here only allow a single line error message)
Honestly, I don't think we need to maintain error-message compatibility with objcopy, where the objcopy message is not as good as we can do.

You should also make sure things like capitalization of error messages etc is consistent with the rest of the code (and ideally the rest of LLVM, although I wouldn't be surprised if there's some variation on a wider scale). Specifically, I think we use upper-case in llvm-objcopy. If this differs from the wider LLVM, we might want to change that, but not in this change.


Repository:
  rL LLVM

https://reviews.llvm.org/D49870