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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please update the llvm-objcopy documentation too.



================
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)
----------------
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?


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


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


================
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)
----------------
My gut says we need something that illustrates this mapping in a test, but I can see people arguing against that.


================
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());
----------------
Same as MaskRay said: needs test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116556