[llvm-objcopy][COFF] Add support for --set-section-flags

[MaskRay]

llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test

2

-o

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

95

How do you get this list?

Have you verified that other flags will be dropped?

[sdmitriev]

llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test

2

Ok.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

95

Well, just by thinking what should be preserved:) I am not completely sure about the IMAGE_SCN_LNK_INFO, but the other flags from this list I think should be there.

[sdmitriev]

Addressed review comments.

[mstorsjo]

Can you check (or did you already?) whether this matches how GNU objcopy behaves (for the cases in GNU objcopy that seem intentional/sensible at least)?

[sdmitriev]

In D73107#1833352, @mstorsjo wrote:

Can you check (or did you already?) whether this matches how GNU objcopy behaves (for the cases in GNU objcopy that seem intentional/sensible at least)?

Yes, I verified that mingw objcopy produces the same results for all tests from llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test except tests error messages and new "exclude" flag. But I did not check what flags it preserves .

[mstorsjo]

In D73107#1833810, @sdmitriev wrote:

In D73107#1833352, @mstorsjo wrote:

Can you check (or did you already?) whether this matches how GNU objcopy behaves (for the cases in GNU objcopy that seem intentional/sensible at least)?

Yes, I verified that mingw objcopy produces the same results for all tests from llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test except tests error messages and new "exclude" flag. But I did not check what flags it preserves .

Ok - as @MaskRay asked about that, can you test which flags are preserved and which dropped implicitly as well?

[sdmitriev]

In D73107#1833870, @mstorsjo wrote:

In D73107#1833810, @sdmitriev wrote:

In D73107#1833352, @mstorsjo wrote:

Can you check (or did you already?) whether this matches how GNU objcopy behaves (for the cases in GNU objcopy that seem intentional/sensible at least)?

Yes, I verified that mingw objcopy produces the same results for all tests from llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test except tests error messages and new "exclude" flag. But I did not check what flags it preserves .

Ok - as @MaskRay asked about that, can you test which flags are preserved and which dropped implicitly as well?

As far as I see it does not preserve any flags at all (except alignment flags). Do you suggest to do the same in llvm-objcopy?

[sdmitriev]

Updated patch to match GNU binutils behavior.

[jhenderson]

The new exclude flag is pretty ELF-centric as things stand. I don't know if there's an equivalent COFF section characteristic that really maps to it. If not, we should either a) reject it, or b) accept it and do nothing with it except perhaps clear the flags. I tentatively prefer b) for simplicity, but regardless it should at least be tested. What do others think?

Documentation should be updated so that --set-section-flags is no longer listed as ELF-specific.

llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test

4

Use '##' for comments to make them stand out better from lit/FileCheck directives. This is what we've tended to do in other new tests for readability.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

220

Any particular reason you're making this const?

[sdmitriev]

In D73107#1836336, @jhenderson wrote:

The new exclude flag is pretty ELF-centric as things stand. I don't know if there's an equivalent COFF section characteristic that really maps to it. If not, we should either a) reject it, or b) accept it and do nothing with it except perhaps clear the flags. I tentatively prefer b) for simplicity, but regardless it should at least be tested. What do others think?

I believe exclude flag which maps to SHF_EXCLUDE for ELF objects maps well to the COFF's IMAGE_SCN_LNK_REMOVE flag.

Documentation should be updated so that --set-section-flags is no longer listed as ELF-specific.

I agree. If documentation says that --set-section-flags is ELF specific it should definitely be updated.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

220

Nothing specific, but I do not see any reasons why it cannot be const especially if we do not plan to modify pointee . Do you see any reasons why it cannot have const qualifier?

[sdmitriev]

Addressed review comments.

[sdmitriev]

Updated documentation change.

[jhenderson]

llvm/docs/CommandGuide/llvm-objcopy.rst

440–441

These two lines need reflowing. We tend to try to keep lines within 80 characters in this document.

440–441

This is still under the "ELF-specific Options" section...

444

I feel like we should also document the effects on COFF objects. Otherwise, people aren't going to know what each setting does.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

220

No specific reason, except that we don't often make local variables const from my experience.

[sdmitriev]

Addressed review comments.

[sdmitriev]

llvm/docs/CommandGuide/llvm-objcopy.rst

440–441

Right. I did not realize that there is a dedicated ELF-specific section. I have added --set-section-flags option description to the COFF-specific section.

[jhenderson]

llvm/docs/CommandGuide/llvm-objcopy.rst

440–441

I've actually just removed the COFF-specific section (see rG0298a8751152). The normal approach would be to put the whole lot in the generic section (since the switch applies to multiple formats), and put the individual ELF and COFF bits one below the other in the description. See --strip-all for a sort-of example (it explains ELF and then COFF/Mach-O).

[sdmitriev]

Updated documentation change.

[jhenderson]

llvm/docs/CommandGuide/llvm-objcopy.rst

143

I'd probably rephrase this as follows:

"For ELF objects, the flags have the following effects:"

I'd also rephrase the COFF one similarly.

158

add -> add the

Here and on each subsequent line starting with "add".

159

unless -> unless the

[sdmitriev]

Addressed comments.

[jhenderson]

Okay, LGTM, but I'd like somebody with more COFF familiarity to confirm they're happy.

[mstorsjo]

LGTM from a COFF perspective

llvm/docs/CommandGuide/llvm-objcopy.rst

  For MachO objects, ``<section>`` must be formatted as
  ``<segment name>,<section name>``.
 
 .. option:: --set-section-alignment <section>=<align>
 
  Set the alignment of section ``<section>`` to `<align>``. Can be specified
  multiple times to update multiple sections.
 
+.. option:: --set-section-flags <section>=<flag>[,<flag>,...]
+
+ Set section properties in the output of section ``<section>`` based on the
+ specified ``<flag>`` values. Can be specified multiple times to update multiple
+ sections.
+
+ Supported flag names are `alloc`, `load`, `noload`, `readonly`, `exclude`,
+ `debug`, `code`, `data`, `rom`, `share`, `contents`, `merge` and `strings`. Not
+ all flags are meaningful for all object file formats.
+
+ For ELF objects, the flags have the following effects:
+
+ - `alloc` = add the `SHF_ALLOC` flag.
+ - `load` = if the section has `SHT_NOBITS` type, mark it as a `SHT_PROGBITS`
+   section.
+ - `readonly` = if this flag is not specified, add the `SHF_WRITE` flag.
+ - `exclude` = add the `SHF_EXCLUDE` flag.
+ - `code` = add the `SHF_EXECINSTR` flag.
+ - `merge` = add the `SHF_MERGE` flag.
+ - `strings` = add the `SHF_STRINGS` flag.
+ - `contents` = if the section has `SHT_NOBITS` type, mark it as a `SHT_PROGBITS`
+   section.
+
+ For COFF objects, the flags have the following effects:
+
+ - `alloc` = add the `IMAGE_SCN_CNT_UNINITIALIZED_DATA` and `IMAGE_SCN_MEM_READ`
+   flags, unless the `load` flag is specified.
+ - `noload` = add the `IMAGE_SCN_LNK_REMOVE` and `IMAGE_SCN_MEM_READ` flags.
+ - `readonly` = if this flag is not specified, add the `IMAGE_SCN_MEM_WRITE`
+   flag.
+ - `exclude` = add the `IMAGE_SCN_LNK_REMOVE` and `IMAGE_SCN_MEM_READ` flags.
+ - `debug` = add the `IMAGE_SCN_CNT_INITIALIZED_DATA`,
+   `IMAGE_SCN_MEM_DISCARDABLE` and  `IMAGE_SCN_MEM_READ` flags.
+ - `code` = add the `IMAGE_SCN_CNT_CODE`, `IMAGE_SCN_MEM_EXECUTE` and
+   `IMAGE_SCN_MEM_READ` flags.
+ - `data` = add the `IMAGE_SCN_CNT_INITIALIZED_DATA` and `IMAGE_SCN_MEM_READ`
+   flags.
+ - `share` = add the `IMAGE_SCN_MEM_SHARED` and `IMAGE_SCN_MEM_READ` flags.
+
 .. option:: --strip-all-gnu
 
  Remove all symbols, debug sections and relocations from the output. This option
  is equivalent to GNU :program:`objcopy`'s ``--strip-all`` switch.
 
 .. option:: --strip-all, -S
 
  For ELF objects, remove from the output all symbols and non-alloc sections not

 
  Preserve access and modification timestamps in the output.
 
 .. option:: --rename-section <old>=<new>[,<flag>,...]
 
  Rename sections called ``<old>`` to ``<new>`` in the output, and apply any
  specified ``<flag>`` values. See :option:`--set-section-flags` for a list of
  supported flags. Can be specified multiple times to rename multiple sections.
 
-.. option:: --set-section-flags <section>=<flag>[,<flag>,...]
-
- Set section properties in the output of section ``<section>`` based on the
- specified ``<flag>`` values. Can be specified multiple times to update multiple
- sections.
-
- Following is a list of supported flags and their effects:
-
- - `alloc` = add the `SHF_ALLOC` flag.
- - `load` = if the section has `SHT_NOBITS` type, mark it as a `SHT_PROGBITS`
-   section.
- - `readonly` = if this flag is not specified, add the `SHF_WRITE` flag.
- - `exclude` = add the `SHF_EXCLUDE` flag.
- - `code` = add the `SHF_EXECINSTR` flag.
- - `merge` = add the `SHF_MERGE` flag.
- - `strings` = add the `SHF_STRINGS` flag.
- - `contents` = if the section has `SHT_NOBITS` type, mark it as a `SHT_PROGBITS`
-   section.
-
- The following flags are also accepted, but are ignored for GNU compatibility:
- `noload`, `debug`, `data`, `rom`, `share`.
-
 .. option:: --set-start-addr <addr>
 
  Set the start address of the output to ``<addr>``. Overrides any previously
  specified :option:`--change-start` or :option:`--adjust-start` options.
 
 .. option:: --split-dwo <dwo-file>
 
  Equivalent to running :program:`llvm-objcopy` with :option:`--extract-dwo` and
  ``<dwo-file>`` as the output file and no other options, and then with
  :option:`--strip-dwo` on the input file.
 
 .. option:: --strip-dwo

llvm/test/tools/llvm-objcopy/COFF/set-section-flags.test

+# RUN: yaml2obj %s -o %t
+
+## Single flag on a section with no flags:
+# RUN: llvm-objcopy --set-section-flags=.foo=alloc %t %t.alloc
+# RUN: llvm-readobj --sections %t.alloc | FileCheck %s --check-prefixes=CHECK,UNINITDATA,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=load %t %t.load
+# RUN: llvm-readobj --sections %t.load | FileCheck %s --check-prefixes=CHECK,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=noload %t %t.noload
+# RUN: llvm-readobj --sections %t.noload | FileCheck %s --check-prefixes=CHECK,READ,WRITE,REMOVE
+# RUN: llvm-objcopy --set-section-flags=.foo=readonly %t %t.readonly
+# RUN: llvm-readobj --sections %t.readonly | FileCheck %s --check-prefixes=CHECK,READ
+# RUN: llvm-objcopy --set-section-flags=.foo=exclude %t %t.exclude
+# RUN: llvm-readobj --sections %t.exclude | FileCheck %s --check-prefixes=CHECK,READ,WRITE,REMOVE
+# RUN: llvm-objcopy --set-section-flags=.foo=debug %t %t.debug
+# RUN: llvm-readobj --sections %t.debug | FileCheck %s --check-prefixes=CHECK,INITDATA,DISCARDABLE,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=code %t %t.code
+# RUN: llvm-readobj --sections %t.code | FileCheck %s --check-prefixes=CHECK,CODE,EXECUTE,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=data %t %t.data
+# RUN: llvm-readobj --sections %t.data | FileCheck %s --check-prefixes=CHECK,INITDATA,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=rom %t %t.rom
+# RUN: llvm-readobj --sections %t.rom | FileCheck %s --check-prefixes=CHECK,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=contents %t %t.contents
+# RUN: llvm-readobj --sections %t.contents | FileCheck %s --check-prefixes=CHECK,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=merge %t %t.merge
+# RUN: llvm-readobj --sections %t.merge | FileCheck %s --check-prefixes=CHECK,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=strings %t %t.strings
+# RUN: llvm-readobj --sections %t.strings | FileCheck %s --check-prefixes=CHECK,READ,WRITE
+# RUN: llvm-objcopy --set-section-flags=.foo=share %t %t.share
+# RUN: llvm-readobj --sections %t.share | FileCheck %s --check-prefixes=CHECK,READ,SHARED,WRITE
+
+## Multiple flags:
+# RUN: llvm-objcopy --set-section-flags=.foo=alloc,readonly,share %t %t.alloc_ro_share
+# RUN: llvm-readobj --sections %t.alloc_ro_share | FileCheck %s --check-prefixes=CHECK,UNINITDATA,READ,SHARED
+# RUN: llvm-objcopy --set-section-flags=.foo=alloc,code %t %t.alloc_code
+# RUN: llvm-readobj --sections %t.alloc_code | FileCheck %s --check-prefixes=CHECK,CODE,UNINITDATA,EXECUTE,READ,WRITE
+
+## Invalid flags:
+# RUN: not llvm-objcopy --set-section-flags=.foo=xyzzy %t %t.xyzzy 2>&1 | FileCheck %s --check-prefix=BAD-FLAG
+
+## Bad flag format:
+# RUN: not llvm-objcopy --set-section-flags=.foo %t %t2 2>&1 | FileCheck %s --check-prefix=BAD-FORMAT
+
+## Setting flags for the same section multiple times:
+# RUN: not llvm-objcopy --set-section-flags=.foo=alloc --set-section-flags=.foo=load %t %t2 2>&1 | FileCheck %s --check-prefix=MULTIPLE-SETS
+
+## Upper-case flags:
+# RUN: llvm-objcopy --set-section-flags=.foo=ALLOC,LOAD,NOLOAD,READONLY,DEBUG,CODE,DATA,ROM,CONTENTS,MERGE,STRINGS,SHARE %t %t.upper
+# RUN: llvm-readobj --sections %t.upper | FileCheck %s --check-prefixes=CHECK,CODE,INITDATA,REMOVE,DISCARDABLE,EXECUTE,READ,SHARED
+
+## Mixed-case flags:
+# RUN: llvm-objcopy --set-section-flags=.foo=aLlOc,LoAd,NoLoad,rEAdONly,Debug,codE,DaTa,rOm,CoNtEnTs,mErGe,sTRINGs,SharE %t %t.mixed
+# RUN: llvm-readobj --sections %t.mixed | FileCheck %s --check-prefixes=CHECK,CODE,INITDATA,REMOVE,DISCARDABLE,EXECUTE,READ,SHARED
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ ]
+sections:
+  - Name:            .foo
+    Characteristics: [ ]
+    Alignment:       4
+symbols:
+...
+
+# CHECK:            Name: .foo
+# CHECK:            Characteristics [
+# CHECK-NEXT:         IMAGE_SCN_ALIGN_4BYTES
+# CODE-NEXT:          IMAGE_SCN_CNT_CODE
+# INITDATA-NEXT:      IMAGE_SCN_CNT_INITIALIZED_DATA
+# UNINITDATA-NEXT:    IMAGE_SCN_CNT_UNINITIALIZED_DATA
+# REMOVE-NEXT:        IMAGE_SCN_LNK_REMOVE
+# DISCARDABLE-NEXT:   IMAGE_SCN_MEM_DISCARDABLE
+# EXECUTE-NEXT:       IMAGE_SCN_MEM_EXECUTE
+# READ-NEXT:          IMAGE_SCN_MEM_READ
+# SHARED-NEXT:        IMAGE_SCN_MEM_SHARED
+# WRITE-NEXT:         IMAGE_SCN_MEM_WRITE
+# CHECK-NEXT:       ]
+
+# BAD-FORMAT: bad format for --set-section-flags: missing '='
+# MULTIPLE-SETS: --set-section-flags set multiple times for section '.foo'
+
+# BAD-FLAG: unrecognized section flag 'xyzzy'. Flags supported for GNU compatibility: alloc, load, noload, readonly, exclude, debug, code, data, rom, share, contents, merge, strings

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 static void addGnuDebugLink(Object &Obj, StringRef DebugLinkFile) {
   std::vector<uint8_t> Contents =
       createGnuDebugLinkSectionContents(DebugLinkFile);
   addSection(Obj, ".gnu_debuglink", Contents,
              IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ |
                  IMAGE_SCN_MEM_DISCARDABLE);
 }
 
+static void setSectionFlags(Section &Sec, SectionFlag AllFlags) {
+  // Need to preserve alignment flags.
+  const uint32_t PreserveMask =
+      IMAGE_SCN_ALIGN_1BYTES | IMAGE_SCN_ALIGN_2BYTES | IMAGE_SCN_ALIGN_4BYTES |
+      IMAGE_SCN_ALIGN_8BYTES | IMAGE_SCN_ALIGN_16BYTES |
+      IMAGE_SCN_ALIGN_32BYTES | IMAGE_SCN_ALIGN_64BYTES |
+      IMAGE_SCN_ALIGN_128BYTES | IMAGE_SCN_ALIGN_256BYTES |
+      IMAGE_SCN_ALIGN_512BYTES | IMAGE_SCN_ALIGN_1024BYTES |
+      IMAGE_SCN_ALIGN_2048BYTES | IMAGE_SCN_ALIGN_4096BYTES |
+      IMAGE_SCN_ALIGN_8192BYTES;
+
+  // Setup new section characteristics based on the flags provided in command
+  // line.
+  uint32_t NewCharacteristics =
+      (Sec.Header.Characteristics & PreserveMask) | IMAGE_SCN_MEM_READ;
+
+  if ((AllFlags & SectionFlag::SecAlloc) && !(AllFlags & SectionFlag::SecLoad))
+    NewCharacteristics |= IMAGE_SCN_CNT_UNINITIALIZED_DATA;
+  if (AllFlags & SectionFlag::SecNoload)
+    NewCharacteristics |= IMAGE_SCN_LNK_REMOVE;
+  if (!(AllFlags & SectionFlag::SecReadonly))
+    NewCharacteristics |= IMAGE_SCN_MEM_WRITE;
+  if (AllFlags & SectionFlag::SecDebug)
+    NewCharacteristics |=
+        IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_DISCARDABLE;
+  if (AllFlags & SectionFlag::SecCode)
+    NewCharacteristics |= IMAGE_SCN_CNT_CODE | IMAGE_SCN_MEM_EXECUTE;
+  if (AllFlags & SectionFlag::SecData)
+    NewCharacteristics |= IMAGE_SCN_CNT_INITIALIZED_DATA;
+  if (AllFlags & SectionFlag::SecShare)
+    NewCharacteristics |= IMAGE_SCN_MEM_SHARED;
+  if (AllFlags & SectionFlag::SecExclude)
+    NewCharacteristics |= IMAGE_SCN_LNK_REMOVE;
+
+  Sec.Header.Characteristics = NewCharacteristics;
+}
+
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   // Perform the actual section removals.
   Obj.removeSections([&Config](const Section &Sec) {
     // Contrary to --only-keep-debug, --only-section fully removes sections that
     // aren't mentioned.
     if (!Config.OnlySection.empty() && !Config.OnlySection.matches(Sec.Name))
       return true;
 
if (!Sym.Referenced) {
           Sym.Sym.StorageClass == IMAGE_SYM_CLASS_STATIC &&
           Sym.Sym.SectionNumber != 0)
         return true;
     }
 
     return false;
   });
 
+  if (!Config.SetSectionFlags.empty())
+    for (Section &Sec : Obj.getMutableSections()) {
+      const auto It = Config.SetSectionFlags.find(Sec.Name);
+      if (It != Config.SetSectionFlags.end())
+        setSectionFlags(Sec, It->second.NewFlags);
+    }
+
   for (const auto &Flag : Config.AddSection) {
     StringRef SecName, FileName;
     std::tie(SecName, FileName) = Flag.split("=");
 
     auto BufOrErr = MemoryBuffer::getFile(FileName);
     if (!BufOrErr)
       return createFileError(FileName, errorCodeToError(BufOrErr.getError()));
     auto Buf = std::move(*BufOrErr);
static Error handleArgs(const CopyConfig &Config, Object &Obj) {
   if (Config.AllowBrokenLinks || !Config.BuildIdLinkDir.empty() ||
       Config.BuildIdLinkInput || Config.BuildIdLinkOutput ||
       !Config.SplitDWO.empty() || !Config.SymbolsPrefix.empty() ||
       !Config.AllocSectionsPrefix.empty() || !Config.DumpSection.empty() ||
       !Config.KeepSection.empty() || Config.NewSymbolVisibility ||
       !Config.SymbolsToGlobalize.empty() || !Config.SymbolsToKeep.empty() ||
       !Config.SymbolsToLocalize.empty() || !Config.SymbolsToWeaken.empty() ||
       !Config.SymbolsToKeepGlobal.empty() || !Config.SectionsToRename.empty() ||
-      !Config.SetSectionAlignment.empty() || !Config.SetSectionFlags.empty() ||
-      Config.ExtractDWO || Config.KeepFileSymbols || Config.LocalizeHidden ||
-      Config.PreserveDates || Config.StripDWO || Config.StripNonAlloc ||
-      Config.StripSections || Config.Weaken || Config.DecompressDebugSections ||
+      !Config.SetSectionAlignment.empty() || Config.ExtractDWO ||
+      Config.KeepFileSymbols || Config.LocalizeHidden || Config.PreserveDates ||
+      Config.StripDWO || Config.StripNonAlloc || Config.StripSections ||
+      Config.Weaken || Config.DecompressDebugSections ||
       Config.DiscardMode == DiscardType::Locals ||
       !Config.SymbolsToAdd.empty() || Config.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for COFF");
   }
 
   return Error::success();
 }