[llvm-objcopy] Implement the PE-COFF specific --subsystem option

[mstorsjo]

Summary

This implements the parsing of the highly PE-COFF specific option
in ConfigManager.cpp, setting Optional<> values in COFFConfig, which
then are used in COFFObjcopy.

This should fix https://github.com/mstorsjo/llvm-mingw/issues/239.

[MaskRay]

llvm/tools/llvm-objcopy/ConfigManager.cpp

758

The diagnostic should be tested.

ELF/ has relatively better testing. See ELF/add-section.test for an example that various cases are tested.

762

A version without . should be tested.

766

The diagnostic should be tested.

[mstorsjo]

llvm/tools/llvm-objcopy/ConfigManager.cpp

598

On the topic of testing diagnostics... Here (in the get*Config() format specific methods) we do error out if a common option has been set, that isn't implemented for that format. But what if options in one of the different format specific structs have been set, e.g. an option set in ELF, but we end up calling getMachOConfig() - then we don't error out at all.

Should we add a separate bool ELFOptionsSet (and similar for all other formats) which we could check for in the get*Config() methods, so we error out cleanly in the same way, if format specific options are set for the wrong format?

[MaskRay]

llvm/tools/llvm-objcopy/ConfigManager.cpp

598

Personally I think no action item is needed for you on this one:)

The diagnostic is untested... I remember that when we add recent new binary-format-specific options, we don't bother add tests for other binary formats. I would not bother adding a diagnostic for non-COFF if I am to implement the feature:)

This part may use some refactoring which can make such tests less necessary, but that will be out of the scope of this patch.

[mstorsjo]

Improve test coverage; test for various error cases, test for specifying only a major version.

[mstorsjo]

llvm/tools/llvm-objcopy/ConfigManager.cpp

598

Yeah it's out of scope for this patch as we don't have it so far, but more as a general question whether we should have it.

Sure, these diagnostics are untested, but I still think they're kinda important (so maybe we should add at least some, but not exhaustive, tests for it?)

But maybe the common options are more important to bail out on, rather than format specific options which one doesn't end up using for incorrect formats quite as easily, contrary to wanting to use unimplemented features.

[avl]

llvm/tools/llvm-objcopy/ConfigManager.cpp

598

It was a design decision that warning message should be displayed for common but not yet implemented option and warning message should NOT be displayed for format specific option set with other formats: https://reviews.llvm.org/D102277#2772972 . That way, using of format specific options when multiple inputs in different formats are specified - would be handled without error messages.

llvm/tools/llvm-objcopy/ObjcopyOpts.td

110

we do not have similar description for other format specific options : "[COFF only]". It looks useful, but probably it would be better to make it as a separate patch for all format specific options.

[mstorsjo]

llvm/tools/llvm-objcopy/ConfigManager.cpp

598

Ok, fair enough.

llvm/tools/llvm-objcopy/ObjcopyOpts.td

110

Ok, I'll remove it from here.

[mstorsjo]

Removed the "COFF only" annotation from the new option.

[jhenderson]

Out of time today, but will look at this tomorrow.

[jhenderson]

Please update the llvm-objcopy documentation too.

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

264

New error messages should use lower-case for their first letter, according to the coding standard.

I don't think I see a test case for this though?

269–270

If I'm reading things right, there's no test for an explicit minor version.

llvm/tools/llvm-objcopy/ConfigManager.cpp

738

Probably should be spelled out rather than using auto? In particular, I believe it might be a pointer, so this needs clarifying.

743–754

My gut says we need something that illustrates this mapping in a test, but I can see people arguing against that.

772

Same as MaskRay said: needs test

[mstorsjo]

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

264

Ok, making it lower case and adding a test case for it.

269–270

That’s right, it’s impossible to set only the minor version but not the major at the same time, via the options. But with the raw API, it would be possible.

llvm/tools/llvm-objcopy/ConfigManager.cpp

738

I can change this into for (const llvm::opt::Arg *Arg. Note that there’s many other identical loops with for (auto Arg : InputArgs.filtered(… within the same function, so this makes it a bit more inconsistent though.

743–754

You mean testing all of the switch values? I think it’s overkill…

772

I did add a test for this one, see line 28 in the current version of the test.

[mstorsjo]

Mention the new option in the docs. Lowercase an error message. Add a test for erroring out on object files. Spell out the type instead of using auto.

[mstorsjo]

Fix a typo in a comment in the new test.

[jhenderson]

llvm/test/tools/llvm-objcopy/COFF/pe-fields.test

8

Should the terminology "object files" actually be "relocatable object files"? At least for ELF, al ELF files are object files, whether executable or not (I don't know if the same applies for COFF though).

Regardless, it would be good if this error message contained the offending file's name.

llvm/test/tools/llvm-objcopy/COFF/subsystem.test

26

This check should include the "foobar" in it somewhere. You might want a {{$}} at the end of this message too, since it's a strict prefix of one of the other errors (and you want to avoid a false pass, where the test is reporting a differenet error).

31

I'd prefer it if this had the explicit "major" or "minor" version. Could be achieved using FileCheck variables, i.e. is not a valid subsystem [[TYPE]] version or similar.

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

263

At least elsewhere in the patch, we don't bother with the explicit llvm:: prefix here.

llvm/tools/llvm-objcopy/ConfigManager.cpp

738

I'd be okay with const auto *Arg if you prefer that for a degree of additional consistency. My main issue is the pointer-ness of Arg. Or, if you are particularly keen on the consistency, I don't mind reverting to what you had before.

[mstorsjo]

llvm/test/tools/llvm-objcopy/COFF/pe-fields.test

8

I guess relocatable object file is more correct for COFF too. I'll include the offending file name and check it in the test.

llvm/test/tools/llvm-objcopy/COFF/subsystem.test

26

Ok, I'll include the incorrect subsystem name in the test pattern. (Then it's no longer a prefix of the other messages, but I can include the {{$}} for good measure in any case.)

31

Ok. I can just split up the patterns for clarity too, and have the check include the unparseable part of the version number too.

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

263

Ok, I'll leave it out here. (This file has a couple existing cases with such redundant prefixes.)

llvm/tools/llvm-objcopy/ConfigManager.cpp

738

Yeah const auto *Arg looks fine to me.

[mstorsjo]

Applied the suggestions.

[jhenderson]

LGTM, with one final suggestion.

llvm/test/tools/llvm-objcopy/COFF/pe-fields.test

6–8

This is how we usually check the file names in error messages.

[mstorsjo]

llvm/test/tools/llvm-objcopy/COFF/pe-fields.test

6–8

Ok, I'll try that. (I was afraid of the risk of extra fragility with potential backslashes as path separators, which can be tricky/require strict correct quoting throughout the tools. But I'll push it through the premerge testing to see that it's ok before landing.)

[mstorsjo]

Check the full exact filename in the error message. Changed to print the output filename instead of the input filename, for the error.

llvm/docs/CommandGuide/llvm-objcopy.rst

 
 MACH-O-SPECIFIC OPTIONS
 -----------------------
 
 .. option:: --keep-undefined
 
  Keep undefined symbols, even if they would otherwise be stripped.
 
+COFF-SPECIFIC OPTIONS
+---------------------
+
+.. option:: --subsystem <name>[:<version>]
+
+ Set the PE subsystem, and optionally subsystem version.
+
 SUPPORTED FORMATS
 -----------------
 
 The following values are currently supported by :program:`llvm-objcopy` for the
 :option:`--input-target`, :option:`--output-target`, and :option:`--target`
 options. For GNU :program:`objcopy` compatibility, the values are all bfdnames.
 
 - `binary`

llvm/test/tools/llvm-objcopy/COFF/pe-fields.test

+## Test that options for altering PE header fields error out on object files.
+
+# RUN: yaml2obj %s -o %t.in.obj
+
+# RUN: not llvm-objcopy --subsystem windows %t.in.obj %t.out.obj 2>&1 | FileCheck %s -DFILE=%t.out.obj
+
+# CHECK: '[[FILE]]': unable to set subsystem on a relocatable object file
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [  ]
+    VirtualAddress:  4096
+    VirtualSize:     1
+    SectionData:     C3
+symbols:
+...

llvm/test/tools/llvm-objcopy/COFF/subsystem.test

+## Test the --subsystem option.
+
+# RUN: yaml2obj %s -o %t.in.exe
+
+# RUN: llvm-objcopy --subsystem=posix:7.4 --subsystem windows %t.in.exe %t.out.exe
+# RUN: llvm-readobj --file-headers %t.out.exe | FileCheck %s --check-prefix=WIN74
+
+# WIN74: MajorOperatingSystemVersion: 6
+# WIN74: MinorOperatingSystemVersion: 0
+# WIN74: MajorSubsystemVersion: 7
+# WIN74: MinorSubsystemVersion: 4
+# WIN74: Subsystem: IMAGE_SUBSYSTEM_WINDOWS_GUI
+
+# RUN: llvm-objcopy --subsystem=posix:7.4 --subsystem windows:9 %t.in.exe %t.out.exe
+# RUN: llvm-readobj --file-headers %t.out.exe | FileCheck %s --check-prefix=WIN90
+
+# WIN90: MajorOperatingSystemVersion: 6
+# WIN90: MinorOperatingSystemVersion: 0
+# WIN90: MajorSubsystemVersion: 9
+# WIN90: MinorSubsystemVersion: 0
+# WIN90: Subsystem: IMAGE_SUBSYSTEM_WINDOWS_GUI
+
+# RUN: not llvm-objcopy --subsystem=foobar %t.in.exe %t.err.exe 2>&1 | FileCheck %s --check-prefix=INVALID-SUBSYS
+
+# INVALID-SUBSYS: 'foobar' is not a valid subsystem{{$}}
+
+# RUN: not llvm-objcopy --subsystem=windows:foo %t.in.exe %t.err.exe 2>&1 | FileCheck %s --check-prefix=INVALID-MAJOR-NUMBER
+# RUN: not llvm-objcopy --subsystem=windows:8.bar %t.in.exe %t.err.exe 2>&1 | FileCheck %s --check-prefix=INVALID-MINOR-NUMBER
+
+# INVALID-MAJOR-NUMBER: 'foo' is not a valid subsystem major version
+# INVALID-MINOR-NUMBER: 'bar' is not a valid subsystem minor version
+
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:       1073741824
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:       IMAGE_SUBSYSTEM_WINDOWS_CUI
+  DLLCharacteristics: [  ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [  ]
+    VirtualAddress:  4096
+    VirtualSize:     1
+    SectionData:     C3
+symbols:
+...

llvm/tools/llvm-objcopy/COFF/COFFConfig.h

 //===- COFFConfig.h ---------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_OBJCOPY_COFF_COFFCONFIG_H
 #define LLVM_TOOLS_LLVM_OBJCOPY_COFF_COFFCONFIG_H
 
+#include "llvm/ADT/Optional.h"
+
 namespace llvm {
 namespace objcopy {
 
 // Coff specific configuration for copying/stripping a single file.
-struct COFFConfig {};
+struct COFFConfig {
+  Optional<unsigned> Subsystem;
+  Optional<unsigned> MajorSubsystemVersion;
+  Optional<unsigned> MinorSubsystemVersion;
+};
 
 } // namespace objcopy
 } // namespace llvm
 
 #endif // LLVM_TOOLS_LLVM_OBJCOPY_COFF_COFFCONFIG_H

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

static uint32_t flagsToCharacteristics(SectionFlag AllFlags, uint32_t OldChar) {
   if (AllFlags & SectionFlag::SecShare)
     NewCharacteristics |= IMAGE_SCN_MEM_SHARED;
   if (AllFlags & SectionFlag::SecExclude)
     NewCharacteristics |= IMAGE_SCN_LNK_REMOVE;
 
   return NewCharacteristics;
 }
 
-static Error handleArgs(const CommonConfig &Config, Object &Obj) {
+static Error handleArgs(const CommonConfig &Config,
+                        const COFFConfig &COFFConfig, 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 (Config.StripDebug || Config.StripAll || Config.StripAllGNU ||
addSection(
                      Buf->getBufferSize()),
         Characteristics);
   }
 
   if (!Config.AddGnuDebugLink.empty())
     if (Error E = addGnuDebugLink(Obj, Config.AddGnuDebugLink))
       return E;
 
+  if (COFFConfig.Subsystem || COFFConfig.MajorSubsystemVersion ||
+      COFFConfig.MinorSubsystemVersion) {
+    if (!Obj.IsPE)
+      return createStringError(
+          errc::invalid_argument,
+          "'" + Config.OutputFilename +
+              "': unable to set subsystem on a relocatable object file");
+    if (COFFConfig.Subsystem)
+      Obj.PeHeader.Subsystem = *COFFConfig.Subsystem;
+    if (COFFConfig.MajorSubsystemVersion)
+      Obj.PeHeader.MajorSubsystemVersion = *COFFConfig.MajorSubsystemVersion;
+    if (COFFConfig.MinorSubsystemVersion)
+      Obj.PeHeader.MinorSubsystemVersion = *COFFConfig.MinorSubsystemVersion;
+  }
+
   return Error::success();
 }
 
-Error executeObjcopyOnBinary(const CommonConfig &Config, const COFFConfig &,
-                             COFFObjectFile &In, raw_ostream &Out) {
+Error executeObjcopyOnBinary(const CommonConfig &Config,
+                             const COFFConfig &COFFConfig, COFFObjectFile &In,
+                             raw_ostream &Out) {
   COFFReader Reader(In);
   Expected<std::unique_ptr<Object>> ObjOrErr = Reader.create();
   if (!ObjOrErr)
     return createFileError(Config.InputFilename, ObjOrErr.takeError());
   Object *Obj = ObjOrErr->get();
   assert(Obj && "Unable to deserialize COFF object");
-  if (Error E = handleArgs(Config, *Obj))
+  if (Error E = handleArgs(Config, COFFConfig, *Obj))
     return createFileError(Config.InputFilename, std::move(E));
   COFFWriter Writer(*Obj, Out);
   if (Error E = Writer.write())
     return createFileError(Config.OutputFilename, std::move(E));
   return Error::success();
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

llvm/tools/llvm-objcopy/ConfigManager.cpp

 //===- ConfigManager.cpp --------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ConfigManager.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/BinaryFormat/COFF.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CRC.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
if (!Common.SplitDWO.empty() || !Common.SymbolsPrefix.empty() ||
       !Common.SymbolsToKeepGlobal.empty() || !Common.SectionsToRename.empty() ||
       !Common.UnneededSymbolsToRemove.empty() ||
       !Common.SetSectionAlignment.empty() || !Common.SetSectionFlags.empty() ||
       Common.ExtractDWO || Common.PreserveDates || Common.StripAllGNU ||
       Common.StripDWO || Common.StripNonAlloc || Common.StripSections ||
       Common.Weaken || Common.DecompressDebugSections || Common.StripUnneeded ||
       Common.DiscardMode == DiscardType::Locals ||
       !Common.SymbolsToAdd.empty()) {
     return createStringError(llvm::errc::invalid_argument,
                              "option not supported by llvm-objcopy for MachO");
   }
 
   return MachO;
 }
 
 Expected<const WasmConfig &> ConfigManager::getWasmConfig() const {
   if (!Common.AddGnuDebugLink.empty() || Common.ExtractPartition ||
if (Positional.empty())
     return createStringError(errc::invalid_argument, "no input file specified");
 
   if (Positional.size() > 2)
     return createStringError(errc::invalid_argument,
                              "too many positional arguments");
 
   ConfigManager ConfigMgr;
   CommonConfig &Config = ConfigMgr.Common;
+  COFFConfig &COFFConfig = ConfigMgr.COFF;
   ELFConfig &ELFConfig = ConfigMgr.ELF;
   MachOConfig &MachOConfig = ConfigMgr.MachO;
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
   if (InputArgs.hasArg(OBJCOPY_target) &&
       (InputArgs.hasArg(OBJCOPY_input_target) ||
        InputArgs.hasArg(OBJCOPY_output_target)))
     return createStringError(
ELFConfig.NewSymbolVisibility = StringSwitch<uint8_t>(VisibilityStr)
                                         .Default(Invalid);
 
     if (ELFConfig.NewSymbolVisibility == Invalid)
       return createStringError(errc::invalid_argument,
                                "'%s' is not a valid symbol visibility",
                                VisibilityStr.str().c_str());
   }
 
+  for (const auto *Arg : InputArgs.filtered(OBJCOPY_subsystem)) {
+    StringRef Subsystem, Version;
+    std::tie(Subsystem, Version) = StringRef(Arg->getValue()).split(':');
+    COFFConfig.Subsystem =
+        StringSwitch<unsigned>(Subsystem.lower())
+            .Case("boot_application",
+                  COFF::IMAGE_SUBSYSTEM_WINDOWS_BOOT_APPLICATION)
+            .Case("console", COFF::IMAGE_SUBSYSTEM_WINDOWS_CUI)
+            .Case("efi_application", COFF::IMAGE_SUBSYSTEM_EFI_APPLICATION)
+            .Case("efi_boot_service_driver",
+                  COFF::IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER)
+            .Case("efi_rom", COFF::IMAGE_SUBSYSTEM_EFI_ROM)
+            .Case("efi_runtime_driver",
+                  COFF::IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER)
+            .Case("native", COFF::IMAGE_SUBSYSTEM_NATIVE)
+            .Case("posix", COFF::IMAGE_SUBSYSTEM_POSIX_CUI)
+            .Case("windows", COFF::IMAGE_SUBSYSTEM_WINDOWS_GUI)
+            .Default(COFF::IMAGE_SUBSYSTEM_UNKNOWN);
+    if (*COFFConfig.Subsystem == COFF::IMAGE_SUBSYSTEM_UNKNOWN)
+      return createStringError(errc::invalid_argument,
+                               "'%s' is not a valid subsystem",
+                               Subsystem.str().c_str());
+    if (!Version.empty()) {
+      StringRef Major, Minor;
+      std::tie(Major, Minor) = Version.split('.');
+      unsigned Number;
+      if (Major.getAsInteger(10, Number))
+        return createStringError(errc::invalid_argument,
+                                 "'%s' is not a valid subsystem major version",
+                                 Major.str().c_str());
+      COFFConfig.MajorSubsystemVersion = Number;
+      Number = 0;
+      if (!Minor.empty() && Minor.getAsInteger(10, Number))
+        return createStringError(errc::invalid_argument,
+                                 "'%s' is not a valid subsystem minor version",
+                                 Minor.str().c_str());
+      COFFConfig.MinorSubsystemVersion = Number;
+    }
+  }
+
   Config.OutputFormat = StringSwitch<FileFormat>(OutputFormat)
                             .Case("binary", FileFormat::Binary)
                             .Case("ihex", FileFormat::IHex)
                             .Default(FileFormat::Unspecified);
   if (Config.OutputFormat == FileFormat::Unspecified) {
     if (OutputFormat.empty()) {
       Config.OutputFormat = Config.InputFormat;
     } else {

llvm/tools/llvm-objcopy/ObjcopyOpts.td

: Eq<"strip-unneeded-symbol",
          "Remove symbol <symbol> if it is not needed by relocations">,
       MetaVarName<"symbol">;
 defm strip_unneeded_symbols
     : Eq<"strip-unneeded-symbols",
          "Reads a list of symbols from <filename> and removes them "
          "if they are not needed by relocations">,
       MetaVarName<"filename">;
 
+defm subsystem
+    : Eq<"subsystem",
+         "Set PE subsystem and version">,
+      MetaVarName<"name[:version]">;
+
 def extract_dwo
     : Flag<["--"], "extract-dwo">,
       HelpText<
           "Remove all sections that are not DWARF .dwo sections from file">;
 
 defm extract_partition
     : Eq<"extract-partition", "Extract named partition from input file">,
       MetaVarName<"name">;