[llvm-objcopy][WebAssembly] Add dump/add/remove-section support

[dschuff]

Summary

Add support for adding, removing, and dumping wasm sections to objcopy.

Depends on D70930

[dschuff]

fix dir name

[dschuff]

• Split tests

[dschuff]

• fix newline

[dschuff]

I believe this patch is now also ready, please take a look.

[jhenderson]

llvm/test/tools/llvm-objcopy/wasm/add-section.test

3

'#' -> '##'

5–6

Where does %t2 come from? I think this test is missing a line to remove the section after the dump?

I'd expect to see something like:

# RUN: llvm-objcopy --dump-section=producers=%t.sec --remove-section=producers %t %t2
# RUN: llvm-objcopy --add-section=producers=%t.sec %t2 %t3

70–74

I generally find it easier to follow the test if CHECKs are before the YAML, especially where the YAML is large.

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

1 ↗

(On Diff #232390)

You need a test showing what happens when you attempt to dump a non-existent section.

24 ↗

(On Diff #232390)

Missing trailing full stop.

25–26 ↗

(On Diff #232390)

Same comment as add-section.test

llvm/test/tools/llvm-objcopy/wasm/remove-section.test

1 ↗

(On Diff #232390)

Missing trailing full stop.

3 ↗

(On Diff #232390)

You need a test showing what happens when you attempt to remove a non-existent section.

23–25 ↗

(On Diff #232390)

Same comment as in other test (move earlier).

Also, this is insufficient to show that the producers section doesn't exist. This only shows that it doesn't exist AFTER the TYPE section. You either want CHECK-NOT lines before and after the CHECK: TYPE line, or you want to add --implicit-check-not=producers to your FileCheck line.

llvm/tools/llvm-objcopy/wasm/Object.cpp

28 ↗

(On Diff #232390)

Nit: missing trailing full stop.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

35

What is the type of Sec here (please don't use auto unless the type is obvious)? Also, should it be a const &?

40–41

If there's an easy way, this should be tested. Example is that the output file is a directory.

44

If there's an easy way, this should be tested too.

62

I'd prefer this not to be auto.

65

Ditto.

75

Ditto.

[dschuff]

llvm/test/tools/llvm-objcopy/wasm/add-section.test

5–6

Yeah, my original version tried to avoid using dump/remove in the test, but this ended up a lot simpler this way.
I updated the test as suggested. Your suggestion doesn't work as-written because section removal happens before section dumping (and I kept it that way for consistency with the other targets), but I just did it in a separate invocation.

llvm/test/tools/llvm-objcopy/wasm/remove-section.test

3 ↗

(On Diff #232390)

This is actually a more interesting question than I realized. The current behavior is to error on dumping a nonexistent section but will silently ignore request to remove nonexistent sections, which matches ELF objcopy. I can see why one might want that behavior, but is that in your opinion a good thing to duplicate from ELF?

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

44

I couldn't find any way to cause error this without trying to do racy things like changing things about the filesystem while llvm-objcopy is running.

[dschuff]

• review comments

[jhenderson]

llvm/test/tools/llvm-objcopy/wasm/add-section.test

5–6

Your suggestion doesn't work as-written because section removal happens before section dumping (and I kept it that way for consistency with the other targets), but I just did it in a separate invocation.

It turns out that this is a bug compared with GNU objcopy for ELF (and I'm guessing other formats too): --dump-section should be runnable in the same operation as the section is removed. I've filed PR44283 for that.

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

7 ↗

(On Diff #233450)

I'd recommend changing this to "%t.dir/bar" or similar, since theoretically something else might create "foo" (by accident or intentionally, it doesn't matter).

14 ↗

(On Diff #233450)

I'd include the bit of the path that provides context in this check - I'm guessing something like error: {{.*}}/bar: {{[nN]}}o such file or directory would work.

llvm/test/tools/llvm-objcopy/wasm/remove-section.test

3 ↗

(On Diff #232390)

Most of the llvm-objcopy behaviour for ELF at least is intended to replicate what GNU objcopy does, for compatibility reasons. I'm guessing there isn't another version of objcopy for wasm yet? If there is, we should try to follow that. Otherwise, I think for consistency it probably makes sense to follow the general guidelines of what ELF does, as if wasm is support is eventually added to GNU objcopy, it's likely to follow its own behaviour for other formats.

7 ↗

(On Diff #233450)

I think you mean here to write llvm-objcopy --remove-section=nonexistent %t?

It's probably a good idea to check that there are no warnings produced as part of this operation.

9–10 ↗

(On Diff #233450)

You probably want to move this to immediately after the corresponding FileCheck run to make it a little clearer what is using it.

12 ↗

(On Diff #233450)

Unnecessary?

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

62

No need for a StringRef to be a const &, since it is already implicitly.

65

clang-format?

75

See my above comment re. StringRef

[dschuff]

• Merge branch 'objcopy-wasm' into objcopy-add-remove
• Make Object own the OwnedContents instead of Section

[dschuff]

• Rebase on master and initial llvm-objcopy patch

[merge_guards_bot]

Unit tests: pass. 62119 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 2 errors and 6 warnings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[dschuff]

• Merge branch 'master' into objcopy-add-remove
• Update for fixes in previous CL

[merge_guards_bot]

Unit tests: fail. 62355 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[dschuff]

• comments, and fix file errors
• dump section before removal

[dschuff]

Rebased against master, and addressed all the comments; please take a look.

llvm/test/tools/llvm-objcopy/wasm/add-section.test

5–6

Fixed for wasm in this patchset, and updated the test as suggested.

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

14 ↗

(On Diff #233450)

This actually exposed a bug; namely, that filename in the error message was that of the input file (%t) instead of the section dump output file (%t/bar). This same bug is seems to be present in the ELF objcopy; my fix is making handleArgs create the FileError itself (with either the input or output file) rather than having its caller executeObjcopyOnBinary always wrap the return value. ELF objcopy has a lot more functionality, I don't know if the same approach would work there.

[merge_guards_bot]

Unit tests: fail. 62355 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[jhenderson]

llvm/test/tools/llvm-objcopy/wasm/add-section.test

2

dumps -> dumps and removes

10–13

The whitespace doesn't need to exactly match what obj2yaml produces, so it might look a little nicer and easier to read by reformatting this slightly:

# CHECK:      Name: producers
# CHECK-NEXT: Tools:
# CHECK-NEXT:   - Name:    clang
# CHECK-NEXT:     Version: 9.0.0

73–75

Nit: I think these last three lines (two blank) can be deleted.

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

32–33 ↗

(On Diff #241593)

You can delete these two lines.

14 ↗

(On Diff #233450)

Seems reasonable. I think the same approach for ELF should work, if you don't mind fixing it up in a separate patch.

llvm/test/tools/llvm-objcopy/wasm/remove-section.test

9 ↗

(On Diff #241593)

It's probably a good idea to check that there are no warnings produced as part of this operation.

I think you missed this part. You can check for warnings by doing something like:
# RUN: llvm-objcopy --remove-section=nonexistent=%t.sec %t 2&>1 | count 0

llvm/tools/llvm-objcopy/wasm/Object.h

32 ↗

(On Diff #241593)

I don't think this comment is useful. It's obvious from the function name that this adds a section.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

54

Check against GNU objcopy for ELF, but I suspect --add-section is applied after removal and dumping. This would allow replacing a section to be done by --remove-section foo --add-section foo=foo.bin or something along those lines.

80

Either make this a TODO, or just remove the line, since the check below shows we don't support OnlySection.

[dschuff]

• address review comments, clang-format

llvm/test/tools/llvm-objcopy/wasm/remove-section.test

9 ↗

(On Diff #241593)

Ah yes I did miss this one.

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

54

Confirmed that GNU objdump does dump, remove, then add. Also added a test here.

[merge_guards_bot]

Unit tests: fail. 62355 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[jhenderson]

Looks good. Perhaps worth a test case for --dump-section + --remove-section for the same section? That feels like an important interaction to me.

llvm/test/tools/llvm-objcopy/wasm/add-section.test

21–22

Nit: delete a space on these two lines so that Type/Name/Payload all line up.

[dschuff]

• Merge branch 'master' into objcopy-add-remove
• Address comment, add test for dump+remove

[merge_guards_bot]

Unit tests: pass. 62516 tests passed, 0 failed and 844 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[dschuff]

Fixed the nit and added the test for dump-section and remove-section. The clang-format bot seems very confused, but I ran clang-format locally.

[jhenderson]

LGTM, once a wasm expert gives the thumbs up.

[sbc100]

llvm/test/tools/llvm-objcopy/wasm/add-section.test

3

Would it be worth checking that the initial and final objects are byte-for-byte identical somehow?

llvm/utils/gn/secondary/llvm/tools/llvm-objcopy/BUILD.gn

75 ↗

(On Diff #242699)

Is it now the expectation that we update .gn files along with cmake? (BTW, do you use gn locally?)

[dschuff]

llvm/test/tools/llvm-objcopy/wasm/add-section.test

3

Currently they aren't identical because they don't encode all of the LEBs identically.
Also obj2yaml puts the reloc.CODE section at the end after the producers section, but it gets added to the end by objcopy. So currently it's not actually a goal to have them be bit-identical. If we wanted we could do that in a future patch.

llvm/utils/gn/secondary/llvm/tools/llvm-objcopy/BUILD.gn

75 ↗

(On Diff #242699)

No, that is explicitly not an expectation (https://github.com/llvm/llvm-project/blob/master/llvm/utils/gn/README.rst). But I do sometimes use GN, so I ended up doing it in this case.

llvm/test/tools/llvm-objcopy/wasm/add-section.test

+## This actually tests dumping, removal, and addition.
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --dump-section=producers=%t.sec %t
+# RUN: llvm-objcopy -R producers %t %t2
+# RUN: llvm-objcopy --add-section=producers=%t.sec %t2 %t3
+# RUN: obj2yaml %t2 | FileCheck --check-prefix=REMOVED %s
+# RUN: obj2yaml %t3 | FileCheck %s
+
+--- !WASM
+FileHeader:
+  Version:         0x00000001
+Sections:
+  - Type:            TYPE
+    Signatures:
+      - Index:           0
+        ParamTypes:
+          - I32
+        ReturnTypes:
+          - F32
+      - Index:           1
+        ParamTypes:
+          - I32
+          - I64
+        ReturnTypes:     []
+  - Type:            FUNCTION
+    FunctionTypes:
+      - 0
+      - 1
+  - Type:            CODE
+    Relocations:
+      - Type:            R_WASM_TABLE_INDEX_SLEB
+        Index:           0
+        Offset:          0x00000006
+      - Type:            R_WASM_FUNCTION_INDEX_LEB
+        Index:           1
+        Offset:          0x00000025
+    Functions:
+      - Index:              0
+        Locals:
+         - Type:            I32
+           Count:           3
+        Body:            418080808000210020002101200111808080800000210220020F0B
+      - Index:              1
+        Locals:
+         - Type:            I32
+           Count:           1
+        Body:            108180808000210020000F0B
+  - Type:            CUSTOM
+    Name:            linking
+    Version:         2
+    SymbolTable:
+      - Index:           0
+        Kind:            FUNCTION
+        Name:            func1
+        Flags:           [  ]
+        Function:        0
+      - Index:           1
+        Kind:            FUNCTION
+        Name:            func2
+        Flags:           [  ]
+        Function:        1
+  - Type:            CUSTOM
+    Name:            producers
+    Tools:
+      - Name:            clang
+        Version:         9.0.0
+
+...
+
+## Check that the producers section has been removed.
+# REMOVED-NOT: producers
+
+## Check that the producers section has been added back unchanged.
+# CHECK:    Name:            producers
+# CHECK-NEXT:    Tools:
+# CHECK-NEXT:      - Name:            clang
+# CHECK-NEXT:        Version:         9.0.0

llvm/tools/llvm-objcopy/wasm/WasmObjcopy.cpp

 
 namespace llvm {
 namespace objcopy {
 namespace wasm {
 
 using namespace object;
 using namespace wasm;
 
+// Adds named custom section with given contents to the object.
+static void addSection(Object &Obj, StringRef Name,
+                       ArrayRef<uint8_t> Contents) {
+  Section Sec;
+  Sec.SectionType = llvm::wasm::WASM_SEC_CUSTOM;
+  Sec.Name = Name;
+  Sec.setOwnedContents(Contents);
+  Obj.addSections(Sec);
+}
+
+static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
+                               Object &Obj) {
+  for (auto &Sec : Obj.getSections()) {
+    if (Sec.Name == SecName) {
+      ArrayRef<uint8_t> Contents = Sec.getContents();
+      Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
+          FileOutputBuffer::create(Filename, Contents.size());
+      if (!BufferOrErr)
+        return BufferOrErr.takeError();
+      std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufferOrErr);
+      std::copy(Contents.begin(), Contents.end(), Buf->getBufferStart());
+      if (Error E = Buf->commit())
+        return E;
+      return Error::success();
+    }
+  }
+  return createStringError(errc::invalid_argument, "section '%s' not found",
+                           SecName.str().c_str());
+}
 
 static Error handleArgs(const CopyConfig &Config, Object &Obj) {
+  // Only support AddSection, DumpSection, RemoveSection.
+  // Don't support OnlySection yet because we only operate on custom sections.
+  Obj.removeSections([&Config](const Section &Sec) {
+    if (Config.ToRemove.matches(Sec.Name))
+      return true;
+    return false;
+  });
+
+  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);
+    addSection(
+        Obj, SecName,
+        makeArrayRef(reinterpret_cast<const uint8_t *>(Buf->getBufferStart()),
+                     Buf->getBufferSize()));
+  }
+
+  for (const auto &Flag : Config.DumpSection) {
+    StringRef SecName;
+    StringRef FileName;
+    std::tie(SecName, FileName) = Flag.split("=");
+    if (Error E = dumpSectionToFile(SecName, FileName, Obj))
+      return E;
+  }
+
   if (!Config.AddGnuDebugLink.empty() || !Config.BuildIdLinkDir.empty() ||
       Config.BuildIdLinkInput || Config.BuildIdLinkOutput ||
       Config.ExtractPartition || !Config.SplitDWO.empty() ||
       !Config.SymbolsPrefix.empty() || !Config.AllocSectionsPrefix.empty() ||
       Config.DiscardMode != DiscardType::None || Config.NewSymbolVisibility ||
       !Config.SymbolsToAdd.empty() || !Config.RPathToAdd.empty() ||
       !Config.OnlySection.empty() || !Config.SymbolsToGlobalize.empty() ||
       !Config.SymbolsToKeep.empty() || !Config.SymbolsToLocalize.empty() ||
       !Config.SymbolsToRemove.empty() ||
       !Config.UnneededSymbolsToRemove.empty() ||
       !Config.SymbolsToWeaken.empty() || !Config.SymbolsToKeepGlobal.empty() ||
       !Config.SectionsToRename.empty() || !Config.SetSectionAlignment.empty() ||
-      !Config.SetSectionFlags.empty() || !Config.SymbolsToRename.empty() ||
-      !Config.ToRemove.empty() || !Config.DumpSection.empty() || !Config.AddSection.empty()) {
+      !Config.SetSectionFlags.empty() || !Config.SymbolsToRename.empty()) {
     return createStringError(
         llvm::errc::invalid_argument,
-        "No flags are supported yet other than basic copying");
+        "Only add-section, dump-section, and remove-section are supported");
   }
   return Error::success();
 }
 
 Error executeObjcopyOnBinary(const CopyConfig &Config,
                              object::WasmObjectFile &In, Buffer &Out) {
   Reader TheReader(In);
   Expected<std::unique_ptr<Object>> ObjOrErr = TheReader.create();