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

[rupprecht]

Summary

Add support for --rename-section flags from gnu objcopy.

Not all flags appear to have an effect for ELF objects, but allowing them would allow easier drop-in replacement. Other unrecognized flags are rejected.

This was only tested by comparing flags printed by "readelf -e <.o>" against the output of gnu vs llvm objcopy, it hasn't been tested to be valid beyond that.

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

121

maybe drop Config from the name ? doesn't seem to provide too much value, although i don't insist

214

I think in any case it should not be in the global namespace,
but more importantly, probably it would be better to use ELF* constants directly rather than introducing this enum.

692

above:
i think now things are getting a bit more complicated and this code deserves to be moved into a helper function (it would parse the passed StringRef and return SectionConfig / Error)

695

size_t + no C-style casts, please

716

std::move is unnecessary here - that struct essentially contains only integral types (pointers / numbers) - it doesn't have a move-ctor, thus std::move will only introduce an unnecessary cast here.

[rupprecht]

Thanks for the review! I'm clearly still ramping up on this codebase :)

tools/llvm-objcopy/llvm-objcopy.cpp

214

I think in any case it should not be in the global namespace,

Moved to the anonymous namespace above (w/ config classes)

but more importantly, probably it would be better to use ELF* constants directly rather than introducing this enum.

This isn't meant to replace the ELF SHF_* constants, it's meant to map to allowed command-line flags.
For instance, "readonly" maps to "unset SHF_WRITE"

692

and return SectionConfig / Error

Done -- although it felt more natural to make it return SectionConfig and just call error() directly if anything happens. Is that OK or would you prefer I return Expected<SectionConfig> and do the error() call up a level in here?

[rupprecht]

Refactored/addressed code review comments

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

214

yea, it's not a replacement, but why do we need this double indirection -
i mean now there is a switch (lines 215 - 228) and there is a chain of "if" statements (lines 255 - 265) -
it looks like the latter would be kinda redundant if ParseSectionName returned the flag (instead of the corresponding enum value) - am i missing smth ?

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

522

here and in the other places:

1. when the type is not obvious we usually don't use auto (for better readability),
2. the names of the variable is kinda inconsistent with the type - the type is SectionRename - i'd name the variable smth like SR

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

522

typo: the name of the variable

[jhenderson]

What is the behaviour of --rename-section when it touches a section with existing flags that don't conflict with the specified value? For example alloc when the section has SHF_EXECINSTR already? Does it preserve those flags? Can multiple modes be specified for the same section (e.g. code + alloc)? I don't think the tests really show these.

test/tools/llvm-objcopy/rename-section-flag.test

41

You could probably make this test a little more explicit, and reduce some of the duplication by using --check-prefixes:

--rename-section=.foo=.bar,alloc | FileCheck %s --check-prefixes=CHECK,ALLOC,WRITE
... --rename-section=.foo=.bar,load | FileCheck %s --check-prefixes=CHECK,WRITE

# CHECK: Name: .bar
# CHECK-NEXT: Type: SHT_PROGBITS
# CHECK-NEXT: Flags [
# ALLOC-NEXT: SHF_ALLOC (0x2)
# WRITE-NEXT: SHF_WRITE (0x1)
# CHECK-NEXT: ]

50

I'm a bit surprised by this one. I haven't checked, but I'd expect load to cause SHF_ALLOC.

tools/llvm-objcopy/llvm-objcopy.cpp

214

This function name is a bit misleading to me, since it doesn't do anything with the section name at all. Please rename it to something like ParseSectionRenameFlags or something equally descriptive.

231

I would say this should be called "ParseRenameSectionValue", not Flag, to avoid confusion with the flags portion of the value passed to the option.

265

Why not assign to this directly in the above set of ifs, rather than go via another variable?

[rupprecht]

What is the behaviour of --rename-section when it touches a section with existing flags that don't conflict with the specified value? For example alloc when the section has SHF_EXECINSTR already? Does it preserve those flags?

It looks like SHF_EXECINSTR is not preserved in that case, e.g.

$ cat /tmp/a.c 
char foo[] = "hello";
$ 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,alloc /tmp/a.o /tmp/a-gnu.o; readelf -S /tmp/a-gnu.o | grep -A 1 .foo
  [ 1] .foo              NOBITS           0000000000000000  00000040
       0000000000000000  0000000000000000  WA       0     0     4

Can multiple modes be specified for the same section (e.g. code + alloc)? I don't think the tests really show these.

Yes, it's comma-delimited. I added a couple tests to show that.

test/tools/llvm-objcopy/rename-section-flag.test

41

Thanks, the test is much more readable now. I wasn't aware of multiple check-prefixes.

50

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

tools/llvm-objcopy/llvm-objcopy.cpp

214

So, I had it written w/o this, and it was pretty ugly. I added a separate type here, and I think it's better for a couple reasons:

• It logically separates command line flags (which are really bfd names) against ELF section types.
• It's not a simple map to ELF flag values. Most of them, sure -- for "alloc", just return SHF_ALLOC and have the caller |= the flag value. But there is no "readonly" flag; instead, there's SHF_WRITE, which is set _unless_ "readonly" is set. You could special case that one -- it's the only outlier here -- but I think it's easier to follow the code when written this way.
• I've just started incrementally working on this, and I'm not convinced that what I have is complete w.r.t. gnu objcopy compatibility (or at least as far as we care about matching it). These might do things besides just SHF_* flags, so having separate types makes that more transparent.

[rupprecht]

• Clean up long test case, fix some naming

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

233

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 ?

[alexander-shaposhnikov]

I've added some inline comments, but otherwise this looks ok to me,
please, wait for James as well.

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

14–15

i think SmallVector should go before STLExtras

[rupprecht]

tools/llvm-objcopy/llvm-objcopy.cpp

14–15

git-clang-format wants it here, unfortunately. Spot checking other LLVM files that include both of these, STLExtras comes first ~95% of the time.

233

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)

[rupprecht]

• Change error messages to match gnu binutils

[jakehehrlich]

LGTM, I had the same confusion James did. I have independently confirmed this is the correct behavior. As much as I dislike this behavior I think we need to match what GNU objcopy does here because this might result in observable differences in behavior (like a segfault if this is run on a ,o file and then linked)

test/tools/llvm-objcopy/rename-section-flag.test

50

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...

[jakehehrlich]

Forgot to accept

[jhenderson]

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.

test/tools/llvm-objcopy/rename-section-flag.test

15

I think this should be "EXEC" (or possibly even "EXECINSTR") not "CODE", just to make it completely clear what we mean.

50

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.

tools/llvm-objcopy/llvm-objcopy.cpp

233

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.

247

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)

251

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?

[jhenderson]

Marking as "Request Changes" to clear the LGTM until my above points about section flags are addressed.

[rupprecht]

In D49870#1179887, @jhenderson wrote:

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.

Here's what I've found from some basic testing of gnu objcopy:

• SHF_ALLOC, SHF_WRITE, SHF_EXECINSTR, SHF_MERGE, SHF_STRINGS: covered already, appear to always be cleared
• SHF_INFO_LINK, SHF_OS_NONCONFORMING, SHF_TLS: appear to be always cleared
• SHF_COMPRESSED, SHF_EXCLUDE, SHF_GROUP, SHF_LINK_ORDER: appear to be preserved
• CPU/processor dependent flags (e.g. SHF_X86_64_LARGE) are preserved

I added a mask so that preserved flags are never changed. It works for the current test cases, but I do plan to do more thorough testing (once this patch, and others, land) to see if this is really good enough.

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.

Done -- I couldn't figure out how to get the prefixes configured to have it all in one test case, so I split off the all-flags test into a separate test file.

[rupprecht]

• Preserve certain SHF_* flags and tidy up error messages

[jhenderson]

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.

test/tools/llvm-objcopy/rename-section-flag-preserved.test

1 ↗

(On Diff #158078)

Since this uses an OS-specific flag, will this work without x86_64 target support? Or do we need a REQUIRES?

3 ↗

(On Diff #158078)

Nit: no need to abbreviate this. Just use "with" explicitly.

52 ↗

(On Diff #158078)

Nit: You should indent this to the same level as the previous line.

tools/llvm-objcopy/ObjcopyOpts.td

32–35

Is this clang-formatted? It looks weird...

[rupprecht]

In D49870#1181835, @jhenderson wrote:

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'm still not able to get SHF_TLS propagated, even with a real object file (not just something artificially created w/ yaml2obj):

$ cat /tmp/t.c
static __thread char tls = 5;
char touch() { return ++tls; }
$ clang -c /tmp/t.c -o /tmp/t.o && bin/llvm-readobj -sections /tmp/t.o
  ...
  Section {
    Index: 4
    Name: .tdata (87)
    Type: SHT_PROGBITS (0x1)
    Flags [ (0x403)
      SHF_ALLOC (0x2)
      SHF_TLS (0x400)
      SHF_WRITE (0x1)
    ]
  ...
$ objcopy --rename-section .tdata=.tdata1,alloc /tmp/t.o /tmp/t2.o && bin/llvm-readobj -sections /tmp/t2.o
  ...
  Section {
    Index: 3
    Name: .tdata1 (38)
    Type: SHT_NOBITS (0x8)
    Flags [ (0x3)
      SHF_ALLOC (0x2)
      SHF_WRITE (0x1)
    ]
  ...

Although note that PROGBITS also changed to NOBITS. Maybe we should just not support renaming sections that having SHF_TLS, SHF_INFO_LINK, or other special flags? This could be reaching into unsupported objcopy territory which gnu objcopy is silently allowing.

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'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.

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.

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.

test/tools/llvm-objcopy/rename-section-flag-preserved.test

1 ↗

(On Diff #158078)

No, it appears this works cross platform; I've separated all flags into a separate test case for clarity. (And it works fine even though the machine I'm testing on is not mips/arm/hex).

[rupprecht]

• Cover full OS/proc flags in test, fix other comments

[jhenderson]

In D49870#1182950, @rupprecht wrote:

In 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.

tools/llvm-objcopy/ObjcopyOpts.td

32–35

I just realised that this is in the .td file, so clang-format might not be applicable, but it looks better now anyway, thanks!

[rupprecht]

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.

That wording sounds good to me. I've updated the errors/tests.

LGTM with those changes.

Thanks for the thorough review!

tools/llvm-objcopy/ObjcopyOpts.td

32–35

Actually, I did use clang-format (forcing it with "git-clang-format --extensions td master") to fix my ugly manual formatting. It's similar enough to C++ that clang-format sometimes works well on small diff regions, although it doesn't seem like it's 100% ready for full-file formatting.

[rupprecht]

Tweak flag message string

test/tools/llvm-objcopy/rename-section-flag.test

+# RUN: yaml2obj %s > %t
+
+# Single flags:
+# RUN: llvm-objcopy --rename-section=.foo=.bar,alloc %t %t.alloc
+# RUN: llvm-readobj -sections %t.alloc | FileCheck %s --check-prefixes=CHECK,ALLOC,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,load %t %t.load
+# RUN: llvm-readobj -sections %t.load | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,noload %t %t.noload
+# RUN: llvm-readobj -sections %t.noload | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,readonly %t %t.readonly
+# RUN: llvm-readobj -sections %t.readonly | FileCheck %s --check-prefixes=CHECK
+# RUN: llvm-objcopy --rename-section=.foo=.bar,debug %t %t.debug
+# RUN: llvm-readobj -sections %t.debug | FileCheck %s --check-prefixes=CHECK,WRITE
+# 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
+# RUN: llvm-readobj -sections %t.data | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,rom %t %t.rom
+# RUN: llvm-readobj -sections %t.rom | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,contents %t %t.contents
+# RUN: llvm-readobj -sections %t.contents | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,merge %t %t.merge
+# RUN: llvm-readobj -sections %t.merge | FileCheck %s --check-prefixes=CHECK,MERGE,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,strings %t %t.strings
+# RUN: llvm-readobj -sections %t.strings | FileCheck %s --check-prefixes=CHECK,STRINGS,WRITE
+# RUN: llvm-objcopy --rename-section=.foo=.bar,share %t %t.share
+# RUN: llvm-readobj -sections %t.share | FileCheck %s --check-prefixes=CHECK,WRITE
+
+# Multiple flags:
+# RUN: llvm-objcopy --rename-section=.foo=.bar,alloc,readonly,strings %t %t.alloc_ro_strings
+# RUN: llvm-readobj -sections %t.alloc_ro_strings | FileCheck %s --check-prefixes=CHECK,ALLOC,STRINGS
+# RUN: llvm-objcopy --rename-section=.foo=.bar,alloc,code %t %t.alloc_code
+# RUN: llvm-readobj -sections %t.alloc_code | FileCheck %s --check-prefixes=CHECK,ALLOC,CODE,WRITE
+
+# Invalid flags:
+# RUN: not llvm-objcopy --rename-section=.foo=.bar,xyzzy %t %t.xyzzy 2>&1 | FileCheck %s --check-prefix=BAD-FLAG
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .foo
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Content:        "c3c3c3c3"
+
+# CHECK: Name: .bar
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# ALLOC-NEXT:     SHF_ALLOC (0x2)
+# CODE-NEXT:      SHF_EXECINSTR (0x4)
+# MERGE-NEXT:     SHF_MERGE (0x10)
+# STRINGS-NEXT:   SHF_STRINGS (0x20)
+# WRITE-NEXT:     SHF_WRITE (0x1)
+# CHECK-NEXT:   ]
+
+# BAD-FLAG: unrecognized section flag 'xyzzy'

test/tools/llvm-objcopy/rename-section.test

 # CHECK: Name: .bar
 # CHECK: SectionData (
 # CHECK-NEXT:   0000: C3C3C3C3
 # CHECK-NEXT: )
 # CHECK: Name: .symtab
 # CHECK: Name: .strtab
 # CHECK: Name: .shstrtab
 
-#BAD-FORMAT: Bad format for --rename-section
-#MULTIPLE-RENAMES: Already have a section rename for .foo
+#BAD-FORMAT: bad format for --rename-section
+#MULTIPLE-RENAMES: Multiple renames of section .foo

tools/llvm-objcopy/ObjcopyOpts.td

defm split_dwo : Eq<"split-dwo">,
                  HelpText<"Equivalent to extract-dwo on the input file to <dwo-file>, then strip-dwo on the input file">;
 defm add_gnu_debuglink : Eq<"add-gnu-debuglink">,
                          MetaVarName<"debug-file">,
                          HelpText<"Add a .gnu_debuglink for <debug-file>">;
 defm remove_section : Eq<"remove-section">,
                       MetaVarName<"section">,
                       HelpText<"Remove <section>">;
 defm rename_section : Eq<"rename-section">,
-                      MetaVarName<"old=new">,
-                      HelpText<"Renames a section from old to new">;
+                      MetaVarName<"old=new[,flag1,...]">,
+                      HelpText<"Renames a section from old to new, optionally with specified flags">;
 defm redefine_symbol : Eq<"redefine-sym">,
                        MetaVarName<"old=new">,
                        HelpText<"Change the name of a symbol old to new">;
 def R : JoinedOrSeparate<["-"], "R">,
         Alias<remove_section>;
 defm keep : Eq<"keep">,
             MetaVarName<"section">,
             HelpText<"Keep <section>">;
 defm only_keep : Eq<"only-keep">,
                  MetaVarName<"section">,
                  HelpText<"Remove all but <section>">;

tools/llvm-objcopy/llvm-objcopy.cpp

 //===- llvm-objcopy.cpp ---------------------------------------------------===//
 //
 //                      The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm-objcopy.h"
 #include "Object.h"
+#include "llvm/ADT/BitmaskEnum.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Object/ArchiveWriter.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ELFTypes.h"

 #undef OPTION
 };
 
 class StripOptTable : public opt::OptTable {
 public:
   StripOptTable() : OptTable(StripInfoTable, true) {}
 };
 
+struct SectionRename {
+  StringRef OriginalName;
+  StringRef NewName;
+  Optional<uint64_t> NewFlags;
+};
+
 struct CopyConfig {
   StringRef OutputFilename;
   StringRef InputFilename;
   StringRef OutputFormat;
   StringRef InputFormat;
   StringRef BinaryArch;
 
   StringRef SplitDWO;
   StringRef AddGnuDebugLink;
   std::vector<StringRef> ToRemove;
   std::vector<StringRef> Keep;
   std::vector<StringRef> OnlyKeep;
   std::vector<StringRef> AddSection;
   std::vector<StringRef> SymbolsToLocalize;
   std::vector<StringRef> SymbolsToGlobalize;
   std::vector<StringRef> SymbolsToWeaken;
   std::vector<StringRef> SymbolsToRemove;
   std::vector<StringRef> SymbolsToKeep;
-  StringMap<StringRef> SectionsToRename;
+  StringMap<SectionRename> SectionsToRename;
   StringMap<StringRef> SymbolsToRename;
   bool StripAll = false;
   bool StripAllGNU = false;
   bool StripDebug = false;
   bool StripSections = false;
   bool StripNonAlloc = false;
   bool StripDWO = false;
   bool StripUnneeded = false;
   bool ExtractDWO = false;
   bool LocalizeHidden = false;
   bool Weaken = false;
   bool DiscardAll = false;
   bool OnlyKeepDebug = false;
   bool KeepFileSymbols = false;
 };
 
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
 
+enum SectionFlag {
+  SecNone = 0,
+  SecAlloc = 1 << 0,
+  SecLoad = 1 << 1,
+  SecNoload = 1 << 2,
+  SecReadonly = 1 << 3,
+  SecDebug = 1 << 4,
+  SecCode = 1 << 5,
+  SecData = 1 << 6,
+  SecRom = 1 << 7,
+  SecMerge = 1 << 8,
+  SecStrings = 1 << 9,
+  SecContents = 1 << 10,
+  SecShare = 1 << 11,
+  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ SecShare)
+};
+
 } // namespace
 
 namespace llvm {
 namespace objcopy {
 
 // The name this program was invoked as.
 StringRef ToolName;
 
LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, Error E) {
   OS.flush();
   errs() << ToolName << ": '" << File << "': " << Buf;
   exit(1);
 }
 
 } // end namespace objcopy
 } // end namespace llvm
 
+static SectionFlag ParseSectionRenameFlag(StringRef SectionName) {
+  return llvm::StringSwitch<SectionFlag>(SectionName)
+      .Case("alloc", SectionFlag::SecAlloc)
+      .Case("load", SectionFlag::SecLoad)
+      .Case("noload", SectionFlag::SecNoload)
+      .Case("readonly", SectionFlag::SecReadonly)
+      .Case("debug", SectionFlag::SecDebug)
+      .Case("code", SectionFlag::SecCode)
+      .Case("data", SectionFlag::SecData)
+      .Case("rom", SectionFlag::SecRom)
+      .Case("merge", SectionFlag::SecMerge)
+      .Case("strings", SectionFlag::SecStrings)
+      .Case("contents", SectionFlag::SecContents)
+      .Case("share", SectionFlag::SecShare)
+      .Default(SectionFlag::SecNone);
+}
+
+static SectionRename ParseRenameSectionValue(StringRef FlagValue) {
+  if (!FlagValue.contains('='))
+    error("bad format for --rename-section");
+
+  // Initial split: ".foo" = ".bar,f1,f2,..."
+  auto Old2New = FlagValue.split('=');
+  SectionRename SR;
+  SR.OriginalName = Old2New.first;
+
+  // Flags split: ".bar" "f1" "f2" ...
+  SmallVector<StringRef, 6> NameAndFlags;
+  Old2New.second.split(NameAndFlags, ',');
+  SR.NewName = NameAndFlags[0];
+
+  if (NameAndFlags.size() > 1) {
+    SectionFlag Flags = SectionFlag::SecNone;
+    for (size_t i = 1; i < NameAndFlags.size(); ++i) {
+      SectionFlag Flag = ParseSectionRenameFlag(NameAndFlags[i]);
+      // FIXME: print the recognized flag values on error.
+      if (Flag == SectionFlag::SecNone)
+        error("unrecognized section flag '" + NameAndFlags[i] + "'");
+      Flags |= Flag;
+    }
+
+    SR.NewFlags = 0;
+    if (Flags & SectionFlag::SecAlloc)
+      *SR.NewFlags |= ELF::SHF_ALLOC;
+    if (!(Flags & SectionFlag::SecReadonly))
+      *SR.NewFlags |= ELF::SHF_WRITE;
+    if (Flags & SectionFlag::SecCode)
+      *SR.NewFlags |= ELF::SHF_EXECINSTR;
+    if (Flags & SectionFlag::SecMerge)
+      *SR.NewFlags |= ELF::SHF_MERGE;
+    if (Flags & SectionFlag::SecStrings)
+      *SR.NewFlags |= ELF::SHF_STRINGS;
+  }
+
+  return SR;
+}
+
 static bool IsDWOSection(const SectionBase &Sec) {
   return Sec.Name.endswith(".dwo");
 }
 
 static bool OnlyKeepDWOPred(const Object &Obj, const SectionBase &Sec) {
   // We can't remove the section header string table.
   if (&Sec == Obj.SectionNames)
     return false;
if ((!Config.SymbolsToKeep.empty() || Config.KeepFileSymbols) &&
     };
   }
 
   Obj.removeSections(RemovePred);
 
   if (!Config.SectionsToRename.empty()) {
     for (auto &Sec : Obj.sections()) {
       const auto Iter = Config.SectionsToRename.find(Sec.Name);
-      if (Iter != Config.SectionsToRename.end())
-        Sec.Name = Iter->second;
+      if (Iter != Config.SectionsToRename.end()) {
+        const SectionRename &SR = Iter->second;
+        Sec.Name = SR.NewName;
+        if (SR.NewFlags.hasValue())
+          Sec.Flags = SR.NewFlags.getValue();
+      }
     }
   }
 
   if (!Config.AddSection.empty()) {
     for (const auto &Flag : Config.AddSection) {
       auto SecPair = Flag.split("=");
       auto SecName = SecPair.first;
       auto File = SecPair.second;
for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbol)) {
     if (!StringRef(Arg->getValue()).contains('='))
       error("Bad format for --redefine-sym");
     auto Old2New = StringRef(Arg->getValue()).split('=');
     if (!Config.SymbolsToRename.insert(Old2New).second)
       error("Multiple redefinition of symbol " + Old2New.first);
   }
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_rename_section)) {
-    if (!StringRef(Arg->getValue()).contains('='))
-      error("Bad format for --rename-section");
-    auto Old2New = StringRef(Arg->getValue()).split('=');
-    if (!Config.SectionsToRename.insert(Old2New).second)
-      error("Already have a section rename for " + Old2New.first);
+    SectionRename SR = ParseRenameSectionValue(StringRef(Arg->getValue()));
+    if (!Config.SectionsToRename.try_emplace(SR.OriginalName, SR).second)
+      error("Multiple renames of section " + SR.OriginalName);
   }
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_remove_section))
     Config.ToRemove.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep))
     Config.Keep.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_only_keep))
     Config.OnlyKeep.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_add_section))
     Config.AddSection.push_back(Arg->getValue());
   Config.StripAll = InputArgs.hasArg(OBJCOPY_strip_all);
   Config.StripAllGNU = InputArgs.hasArg(OBJCOPY_strip_all_gnu);
   Config.StripDebug = InputArgs.hasArg(OBJCOPY_strip_debug);
   Config.StripDWO = InputArgs.hasArg(OBJCOPY_strip_dwo);
   Config.StripSections = InputArgs.hasArg(OBJCOPY_strip_sections);
   Config.StripNonAlloc = InputArgs.hasArg(OBJCOPY_strip_non_alloc);
   Config.StripUnneeded = InputArgs.hasArg(OBJCOPY_strip_unneeded);
   Config.ExtractDWO = InputArgs.hasArg(OBJCOPY_extract_dwo);
   Config.LocalizeHidden = InputArgs.hasArg(OBJCOPY_localize_hidden);
   Config.Weaken = InputArgs.hasArg(OBJCOPY_weaken);
   Config.DiscardAll = InputArgs.hasArg(OBJCOPY_discard_all);
   Config.OnlyKeepDebug = InputArgs.hasArg(OBJCOPY_only_keep_debug);
   Config.KeepFileSymbols = InputArgs.hasArg(OBJCOPY_keep_file_symbols);
   for (auto Arg : InputArgs.filtered(OBJCOPY_localize_symbol))
     Config.SymbolsToLocalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_globalize_symbol))
     Config.SymbolsToGlobalize.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbol))
     Config.SymbolsToWeaken.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbol))
     Config.SymbolsToRemove.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     Config.SymbolsToKeep.push_back(Arg->getValue());