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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked an inline comment as done.
mstorsjo added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:264
+      return createStringError(llvm::errc::invalid_argument,
+                               "Unable to set subsystem on object file");
+    if (COFFConfig.Subsystem)
----------------
jhenderson wrote:
> 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?
Ok, making it lower case and adding a test case for it.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:269-270
+      Obj.PeHeader.MajorSubsystemVersion = *COFFConfig.MajorSubsystemVersion;
+    if (COFFConfig.MinorSubsystemVersion)
+      Obj.PeHeader.MinorSubsystemVersion = *COFFConfig.MinorSubsystemVersion;
+  }
----------------
jhenderson wrote:
> If I'm reading things right, there's no test for an explicit minor version.
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.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:738
 
+  for (auto Arg : InputArgs.filtered(OBJCOPY_subsystem)) {
+    StringRef Subsystem, Version;
----------------
jhenderson wrote:
> Probably should be spelled out rather than using `auto`? In particular, I believe it might be a pointer, so this needs clarifying.
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.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:743-754
+            .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)
----------------
jhenderson wrote:
> My gut says we need something that illustrates this mapping in a test, but I can see people arguing against that.
You mean testing all of the switch  values? I think it’s overkill…


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:772
+        return createStringError(errc::invalid_argument,
+                                 "'%s' is not a valid subsystem minor version",
+                                 Minor.str().c_str());
----------------
jhenderson wrote:
> Same as MaskRay said: needs test
I did add a test for this one, see line 28 in the current version of the test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116556/new/

https://reviews.llvm.org/D116556