ObjCopy: support `--dump-section` on COFF

[compnerd]

Summary

Add support for --dump-section on COFF files. This is helpful for
extracting specific content from an object file on Windows.

[MaskRay]

@hjyamauchi If you want to change username, I can help. Do you want to rename yamauchi to another name (which one?) and rename hjyamauchi to yamauchi?
There may also be ways to find passwords for yamauchi.

[alexander-shaposhnikov]

minor nits inline, otherwise looks good to me.

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

145

nit:

llvm::copy(Contents, Buffer->getBufferStart());

160

nit:

auto [Section, File] = Op.split('=');

[jhenderson]

Requesting changes, just to stop this landing before my comments have been addressed. I don't see any serious issues, just a couple of stylistic things and some missing testing.

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

135

This doesn't look to conform to standard LLVM coding style. Please make sure you have run clang-format on your code.

152

Please see the coding standards for errors and warning messages (use lower-case "s" for this message): https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

llvm/test/tools/llvm-objcopy/COFF/dump-section.test

2

Please add the following two test cases: 1) the section does not exist; 2) the file cannot be created (e.g. because you're trying to create a file with the same path as an existing directory).

Check that llvm-objcopy emits an error with the expected message in these cases.

It may also be interesting to show that the contents of an existing file on disk are overwritten if it is the target of --dump-section.

17

I'm not too familiar with coff yaml2obj, so my comments may not be applicable, but can you drop most of these sections from the YAML? I think you only need two sections (one empty and one non-empty). If possible, I'd even name those sections accordingly (i.e. something like ".empty" and ".nonempty"), and remove their characteristics, since they aren't relevant to the behaviour of llvm-objcopy.

43

Similar to the above, if you can, please remove all the symbols (or at least the ones that aren't required).

[compnerd]

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

135

It does - it just is the older style :). Old habits die hard. I had avoided running it as it results in a lot of noise due to the removal, but I've run it and it does reflow this (and the other file - which is why I had avoided it.

152

Probably should do the same in the ELF path :)

llvm/test/tools/llvm-objcopy/COFF/dump-section.test

2

Should probably add those to the ELF tests as well :)

[compnerd]

Address feedback

[compnerd]

Fix for cross-platform testing

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

133

nit: "S" -> "Section" to match "File"?

152

nit: Would it be good to match the ELF version with "object_error::parse_failed"? Or the other way around? Perhaps doesn't matter.

In D150305#4333439, @MaskRay wrote:

@hjyamauchi If you want to change username, I can help. Do you want to rename yamauchi to another name (which one?) and rename hjyamauchi to yamauchi?
There may also be ways to find passwords for yamauchi.

Hi Ray, thanks for the offer for help. I created the second account 'hjyamauchi' with the goal of integrating it with the login with my github account hjyamauchi@ and matching the github username and the phabricator username. Maybe that was my confusion around the github migration and it'd have been possible to switch an existing username/password-based account to a github login? I can log into both accounts. If possible, it might be best to rename 'yamauchi' to 'hjyamauchi' (and remove or rename the current 'hjyamauchi'), keep the review history from 'yamauchi' and integrate that with my github login.

[compnerd]

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

133

That collides with the iterator - I can use SectionName but that also is different from File ... I guess I can do SectionName and FileName.

152

Ugh! That didn't match what I saw locally. I guess I was out of date :-(. Thanks!

[MaskRay]

In D150305#4335987, @hjyamauchi-test wrote:

In D150305#4333439, @MaskRay wrote:

@hjyamauchi If you want to change username, I can help. Do you want to rename yamauchi to another name (which one?) and rename hjyamauchi to yamauchi?
There may also be ways to find passwords for yamauchi.

Hi Ray, thanks for the offer for help. I created the second account 'hjyamauchi' with the goal of integrating it with the login with my github account hjyamauchi@ and matching the github username and the phabricator username. Maybe that was my confusion around the github migration and it'd have been possible to switch an existing username/password-based account to a github login? I can log into both accounts. If possible, it might be best to rename 'yamauchi' to 'hjyamauchi' (and remove or rename the current 'hjyamauchi'), keep the review history from 'yamauchi' and integrate that with my github login.

Done! Matching the GitHub account is a good reason and I know others renaming for this reason.

Renamed to hjyamauchi to hjyamauchi-test and renamed yamauchi to hjyamauchi :)
@hjyamauchi contains all the contribution history (all occurrences of @yamauchi in old messages are replaced with @hjyamauchi).

If the result looks good, I can disable hjyamauchi-test so that people will not pick the wrong user.
(Like that I had @maskray0 which has been disabled.)

[hjyamauchi]

In D150305#4336082, @MaskRay wrote:

In D150305#4335987, @hjyamauchi-test wrote:

In D150305#4333439, @MaskRay wrote:

@hjyamauchi If you want to change username, I can help. Do you want to rename yamauchi to another name (which one?) and rename hjyamauchi to yamauchi?
There may also be ways to find passwords for yamauchi.

Hi Ray, thanks for the offer for help. I created the second account 'hjyamauchi' with the goal of integrating it with the login with my github account hjyamauchi@ and matching the github username and the phabricator username. Maybe that was my confusion around the github migration and it'd have been possible to switch an existing username/password-based account to a github login? I can log into both accounts. If possible, it might be best to rename 'yamauchi' to 'hjyamauchi' (and remove or rename the current 'hjyamauchi'), keep the review history from 'yamauchi' and integrate that with my github login.

Done! Matching the GitHub account is a good reason and I know others renaming for this reason.

Renamed to hjyamauchi to hjyamauchi-test and renamed yamauchi to hjyamauchi :)
@hjyamauchi contains all the contribution history (all occurrences of @yamauchi in old messages are replaced with @hjyamauchi).

If the result looks good, I can disable hjyamauchi-test so that people will not pick the wrong user.
(Like that I had @maskray0 which has been disabled.)

Thanks! I'll see how the new 'hjyamauchi' works out and let you know. I'll also see if I can integrate it with my github login.

[jhenderson]

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

133

+1 to SectionName and FileName. I think this nicely improves clarity.

152

By all means do so (in a separate patch). I no longer directly work on LLVM, due to a change in job roles. I just review stuff :)

llvm/test/tools/llvm-objcopy/COFF/dump-section.test

2

Same comment as above :)

7–8

Consistency fixes, and no need to ignore the case. Instead, use the %errc... substitutions, if needed (you might need to extend the list of available ones to cover the "is a directory" case).

Also %T is considered deprecated (from the docs: "%T parent directory of %t (not unique, deprecated, do not use)"). You can use %S instead in this case.

14–16

Could you check the full error message, please (less any tool name), using FileCheck -D to parameterise any file names. In particular, this will show that this is an error, and not a warning.

[hjyamauchi]

llvm/lib/ObjCopy/COFF/COFFObjcopy.cpp

133

+1

[compnerd]

I'll make those adjustments and then commit - I'd like to get this through. I can address further feedback in follow ups as it seems most of the comments are about stylistic things at this point.

llvm/test/tools/llvm-objcopy/COFF/dump-section.test

7–8

%T is the correct thing here; %S is the parent of the source which would write the file into the source tree. Creating a directory with %t is possible but annoying.

14–16

Sure.

[compnerd]

Address feedback

[jhenderson]

I'll make those adjustments and then commit - I'd like to get this through. I can address further feedback in follow ups as it seems most of the comments are about stylistic things at this point.

What was the desperate rush about getting this landed without giving it at least 1 working day for other reviewers to have a final check through, especially when you actively disagreed with one of the requests?

llvm/test/tools/llvm-objcopy/COFF/dump-section.test

7–8

The parent of the source will be a directory, which is all you're trying to achieve here. The docs state that using %T is deprecated. You shouldn't use it in new tests. Please address this point.

It looks like you didn't adopt my inline request to use double-dash for --check-prefix: as things stand, you've got FileCheck commands in this test using both single and double-dash (I'm pretty sure double-dash is preferred everwhere).

14–16

This comment was meant to apply to both error cases: please update CHECK-NO-SECTION too.

[MaskRay]

llvm/test/tools/llvm-objcopy/COFF/dump-section.test

7–8

I agree that %T should be avoided. If another test in .../COFF/ does something with %T, this will easily cause a race condition. If a test file creates a lot of temporary files, sometimes # RUN: rm -rf %t && mkdir %t && cd %t is easier.

--check-prefix= is the most common form. -check-prefix= is less recommended but common as well.
-check-prefix is very uncommon and probably should be avoided.

[hjyamauchi]

In D150305#4336273, @hjyamauchi wrote:

In D150305#4336082, @MaskRay wrote:

In D150305#4335987, @hjyamauchi-test wrote:

In D150305#4333439, @MaskRay wrote:

@hjyamauchi If you want to change username, I can help. Do you want to rename yamauchi to another name (which one?) and rename hjyamauchi to yamauchi?
There may also be ways to find passwords for yamauchi.

Hi Ray, thanks for the offer for help. I created the second account 'hjyamauchi' with the goal of integrating it with the login with my github account hjyamauchi@ and matching the github username and the phabricator username. Maybe that was my confusion around the github migration and it'd have been possible to switch an existing username/password-based account to a github login? I can log into both accounts. If possible, it might be best to rename 'yamauchi' to 'hjyamauchi' (and remove or rename the current 'hjyamauchi'), keep the review history from 'yamauchi' and integrate that with my github login.

Done! Matching the GitHub account is a good reason and I know others renaming for this reason.

Renamed to hjyamauchi to hjyamauchi-test and renamed yamauchi to hjyamauchi :)
@hjyamauchi contains all the contribution history (all occurrences of @yamauchi in old messages are replaced with @hjyamauchi).

If the result looks good, I can disable hjyamauchi-test so that people will not pick the wrong user.
(Like that I had @maskray0 which has been disabled.)

Thanks! I'll see how the new 'hjyamauchi' works out and let you know. I'll also see if I can integrate it with my github login.

Hi Ray, I see that the 'hjyamauchi-test' account is already disabled and I cannot log into it anymore. That's fine but would you be able to move the email address associated with the disabled account to the 'hjyamauchi' account (or give it a bonus address, or swap the two gmail accounts in those accounts if an empty address isn't allowed in a disabled account, or delete the disabled account completely so that I can use the address in the 'hjyamauchi' account)? The system doesn't let me do so because the address appears to be still associated with the disabled account. I was going to do this swap on my own earlier but failed to do so yet. Thanks for your help!

llvm/lib/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 dumpSection(Object &O, StringRef SectionName, StringRef FileName) {
+  for (const coff::Section &Section : O.getSections()) {
+    if (Section.Name != SectionName)
+      continue;
+
+    ArrayRef<uint8_t> Contents = Section.getContents();
+
+    std::unique_ptr<FileOutputBuffer> Buffer;
+    if (auto B = FileOutputBuffer::create(FileName, Contents.size()))
+      Buffer = std::move(*B);
+    else
+      return B.takeError();
+
+    llvm::copy(Contents, Buffer->getBufferStart());
+    if (Error E = Buffer->commit())
+      return E;
+
+    return Error::success();
+  }
+  return createStringError(object_error::parse_failed, "section '%s' not found",
+                           SectionName.str().c_str());
+}
+
 static Error handleArgs(const CommonConfig &Config,
                         const COFFConfig &COFFConfig, Object &Obj) {
+  for (StringRef Op : Config.DumpSection) {
+    auto [Section, File] = Op.split('=');
+    if (Error E = dumpSection(Obj, Section, File))
+      return E;
+  }
+
   // 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 ||

llvm/lib/ObjCopy/ConfigManager.cpp

 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 
 namespace llvm {
 namespace objcopy {
 
 Expected<const COFFConfig &> ConfigManager::getCOFFConfig() const {
   if (!Common.SplitDWO.empty() || !Common.SymbolsPrefix.empty() ||
-      !Common.AllocSectionsPrefix.empty() || !Common.DumpSection.empty() ||
-      !Common.KeepSection.empty() || !Common.SymbolsToGlobalize.empty() ||
-      !Common.SymbolsToKeep.empty() || !Common.SymbolsToLocalize.empty() ||
-      !Common.SymbolsToWeaken.empty() || !Common.SymbolsToKeepGlobal.empty() ||
-      !Common.SectionsToRename.empty() || !Common.SetSectionAlignment.empty() ||
-      !Common.SetSectionType.empty() || Common.ExtractDWO ||
-      Common.PreserveDates || Common.StripDWO || Common.StripNonAlloc ||
-      Common.StripSections || Common.Weaken || Common.DecompressDebugSections ||
+      !Common.AllocSectionsPrefix.empty() || !Common.KeepSection.empty() ||
+      !Common.SymbolsToGlobalize.empty() || !Common.SymbolsToKeep.empty() ||
+      !Common.SymbolsToLocalize.empty() || !Common.SymbolsToWeaken.empty() ||
+      !Common.SymbolsToKeepGlobal.empty() || !Common.SectionsToRename.empty() ||
+      !Common.SetSectionAlignment.empty() || !Common.SetSectionType.empty() ||
+      Common.ExtractDWO || Common.PreserveDates || Common.StripDWO ||
+      Common.StripNonAlloc || Common.StripSections || Common.Weaken ||
+      Common.DecompressDebugSections ||
       Common.DiscardMode == DiscardType::Locals || !Common.SymbolsToAdd.empty())
     return createStringError(llvm::errc::invalid_argument,
                              "option is not supported for COFF");
 
   return COFF;
 }
 
 Expected<const MachOConfig &> ConfigManager::getMachOConfig() const {

llvm/test/tools/llvm-objcopy/COFF/dump-section.test

+# RUN: yaml2obj %s -o %t.obj
+# RUN: llvm-objcopy --dump-section .data=%t.dat %t.obj
+# RUN: wc -c %t.dat | FileCheck %s --ignore-case -check-prefix CHECK-EMPTY-SIZE
+# RUN: llvm-objcopy --dump-section .text.f=%t.txt %t.obj
+# RUN: od -t x1 %t.txt | FileCheck %s --ignore-case -check-prefix CHECK-TEXT-F
+# RUN: not llvm-objcopy --dump-section non-existent=/dev/null %t.obj 2>&1 | FileCheck %s -check-prefix CHECK-NO-SECTION
+# RUN: not llvm-objcopy --dump-section .text=%T %t.obj 2>&1 | FileCheck -DOBJ=%t.obj -DMSG=%errc_EISDIR %s -check-prefix CHECK-INVALID-DESTINATION
+
+# CHECK-EMPTY-SIZE: 0
+
+# CHECK-TEXT-F: 0000000 b8 20 00 00 00 c3
+
+# CHECK-NO-SECTION: section 'non-existent' not found
+
+# CHECK-INVALID-DESTINATION: error: '[[OBJ]]': [[MSG]]
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ ]
+    Alignment:       4
+    SectionData:     ''
+  - Name:            .data
+    Characteristics: [ ]
+    Alignment:       4
+    SectionData:     ''
+  - Name:            .text.f
+    Characteristics: [ ]
+    Alignment:       16
+    SectionData:     B820000000C3
+symbols:
+...