[Objcopy][Wasm] Allow selecting known sections by name

[dschuff]

Summary

Currently, only custom sections can be selected by operations that use section
names, because only custom sections have explicit names (whereas known sections
have names defined by the spec and only use their indices in the binary format).
This CL makes objdopy use the spec-defined names for these sections, allowing
them to be used in operations such as dumping and removal.

This is a prerequisite for fixing
https://github.com/emscripten-core/emscripten/issues/13084

[sbc100]

llvm/lib/BinaryFormat/Wasm.cpp

74 ↗

(On Diff #432420)

Looks like should be able to delete the copy of this function from lld/wasm/OutputSections.cpp?

And what looks like another version in WasmObjectFile::getSectionName

[sbc100]

llvm/include/llvm/BinaryFormat/Wasm.h

475 ↗

(On Diff #432420)

Seems like we should be consistent here with std::string vs StringRef.

[dschuff]

Deduplicate

llvm/include/llvm/BinaryFormat/Wasm.h

475 ↗

(On Diff #432420)

agreed, although I think StringRef is probably the better one to use if we can. Also it looks like we have duplicate relocTypetoStrings too, so maybe that's better cleaned up in a separate CL.

llvm/lib/BinaryFormat/Wasm.cpp

74 ↗

(On Diff #432420)

oops yes, I saw the one in lld but forgot to remove it. (didn't see the one in WasmObjectFile)

[dschuff]

llvm/lib/Object/WasmObjectFile.cpp

1732 ↗

(On Diff #432431)

This is ugly now because it "knows" that TAG is the last known section id, which will be wrong when we add another. Do you prefer a fix of adding some kind of LAST_SECTION_ID in BinaryFormat/Wasm.h, maybe a function like isValidSectionId()?

[jhenderson]

Looks like there's no test case?

[sbc100]

llvm/lib/Object/WasmObjectFile.cpp

1732 ↗

(On Diff #432431)

Its still better than the status quo of duplicating the entire function. Are you planning on landing the refactoring/de-duplication change first/separately?

[dschuff]

• Merge branch 'sectionTypeToString' into objcopy-known

[dschuff]

In D126509#3541775, @jhenderson wrote:

Looks like there's no test case?

Sorry, my second diff accidentally included just the refactoring. I opened https://reviews.llvm.org/D126553 for that, and this change is now just the tool change on top of that.

[dschuff]

• Merge branch 'main' into objcopy-known

[dschuff]

OK, the prerequisite refactoring has landed, and this patch is now based on main, please take a look.

[sbc100]

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

24

Just only empty line here?

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

13

Why use --implicit-check-not instead of CHECK-NOT lines. (I've never seen implicit-check-not used before).

[dschuff]

• remove stray blank line

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

13

implicit-check-not ensures that the text doesn't appear anywhere in the file at all. It's like having a CHECK-NOT in between all your CHECK lines, so it's much cleaner for that use case. In this particular case maybe it's not strictly necessary (since the type section could only go before the producers section) but it's used in several o the other objcopy tests, so it's consistent to use it here too.

[dschuff]

@jhenderson or others, would you like to review this too, or is sbc's review sufficient?

[jhenderson]

In D126509#3554020, @dschuff wrote:

@jhenderson or others, would you like to review this too, or is sbc's review sufficient?

I'm happy for @sbc100 to review wasm-only changes like this patch, without contributing myself. If the change starts impacting generic llvm-objcopy code, I'd like to review it though.

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

16

[aheejin]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

30

Maybe create WASM_LAST_SEC or something?

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

17

[dschuff]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

30

Yeah, I noticed that we don't have that anywhere else in BinaryFormat/Wasm.h but you're right that it's used elsewhere in LLVM. Maybe we should add it for this case anyway. There are 2 other places in the code where we compare this way and it does seem like it's a bug waiting to happen. I'll go ahead and do that in a followup.

[dschuff]

In D126509#3559905, @jhenderson wrote:

I'm happy for @sbc100 to review wasm-only changes like this patch, without contributing myself. If the change starts impacting generic llvm-objcopy code, I'd like to review it though.

Thanks I'll keep that in mind.

[aheejin]

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

17

Not something important, but gentle ping?

[dschuff]

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

17

oh, oops, sorry I missed that. Will fix.

[aheejin]

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

17

# CHECK-NOT:    - Type: TYPE

is still missing. Not important though.

[dschuff]

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

17

oops, I completely misread your comment, sorry about that 😓
Yes, I think having that check is a good idea.
It's a little weird because the FileCheck line that includes KEEPTYPE also includes CHECK as a prefix. So in that case KEEPTYPE line requires that line, and then the CHECK-NOT isn't needed. but it does still work.
Maybe even better would be to add --implicit-check-not=TYPE to ensure that the type section isn't anywhere in the file. Although that would be overkill since the line where this CHECK is, is the only place it could appear.
I'll just add the CHECK-NOT where you suggested.

[aheejin]

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

17

Oh, I didn't notice KEEPTYPE has also CHECK in the command. Then does my CHECK-NOT line work? I guess not but you said it works... Anyway nevermind if it doesn't work.

[dschuff]

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

17

It does work; I think it just checks that there isn't another instance of TYPE after it matches the one that's there. So that's not really necessary (in the sense that having the type section be duplicated seems an unlikely failure mode) but doesn't seem harmful either.

llvm/lib/ObjCopy/wasm/WasmReader.cpp

Expected<std::unique_ptr<Object>> Reader::create() const {
   auto Obj = std::make_unique<Object>();
   Obj->Header = WasmObj.getHeader();
   std::vector<Section> Sections;
   Obj->Sections.reserve(WasmObj.getNumSections());
   for (const SectionRef &Sec : WasmObj.sections()) {
     const WasmSection &WS = WasmObj.getWasmSection(Sec);
     Obj->Sections.push_back(
         {static_cast<uint8_t>(WS.Type), WS.Name, WS.Content});
+    // Give known sections standard names to allow them to be selected.
+    Section &ReaderSec = Obj->Sections.back();
+    if (ReaderSec.SectionType > WASM_SEC_CUSTOM &&
+        ReaderSec.SectionType <= WASM_SEC_TAG)
+      ReaderSec.Name = sectionTypeToString(ReaderSec.SectionType);
+    // If the section type is CUSTOM, it has a name already. If it's a new type
+    // of section that we don't explicitly handle here, it will have an empty
+    // name and objcopy won't be able to select it by name (e.g. for removal
+    // or dumping) but it will still be valid and able to be copied.
   }
   return std::move(Obj);
 }
 
 } // end namespace wasm
 } // end namespace objcopy
 } // end namespace llvm

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

 ## Test the contents of a custom section dumped from a binary.
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-objcopy --dump-section=producers=%t.sec %t
 # RUN: od -t x1 %t.sec | FileCheck %s
 
 # RUN: not llvm-objcopy --dump-section=nonexistent=%t.sec %t 2>&1 | FileCheck --check-prefix=NONEXISTENT %s
 # RUN: not llvm-objcopy --dump-section=producers=%t.dir/bar %t 2>&1 | FileCheck -DMSG=%errc_ENOENT --check-prefix=DIROUT %s
 
 ## Raw contents of the producers section.
 # CHECK: 0000000 01 0c 70 72 6f 63 65 73 73 65 64 2d 62 79 01 05
 # CHECK: 0000020 63 6c 61 6e 67 05 39 2e 30 2e 30
 
 # NONEXISTENT: section 'nonexistent' not found
 # DIROUT: error: {{.*}}/bar': [[MSG]]
 
+## Test dumping the type section (a known section)
+# RUN: llvm-objcopy --dump-section=TYPE=%t.sec %t
+# RUN: od -t x1 %t.sec | FileCheck %s --check-prefix=TYPESEC
+
+## Raw contents of the type section.
+# TYPESEC: 000000 01 60 01 7f 01 7d
+# TYPESEC: 000006
+
 ## Check that dumping and removing a section works in the same invocation
 # RUN: llvm-objcopy --dump-section=producers=%t.sec --remove-section=producers %t %t2
 # RUN: od -t x1 %t.sec | FileCheck %s
 # RUN: obj2yaml %t2 | FileCheck --check-prefix=REMOVED %s
 
 # REMOVED-NOT: producers
 
 --- !WASM
 FileHeader:

llvm/test/tools/llvm-objcopy/wasm/only-keep-debug.test

 ## Test that only debug sections are kept with --only-keep-debug.
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-strip --only-keep-debug %t
 # RUN: obj2yaml %t | FileCheck %s
 
 ## Test that keep-section overrides only-keep-debug.
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-strip --only-keep-debug --keep-section=foo %t
 # RUN: obj2yaml %t | FileCheck --implicit-check-not=Name --check-prefix=CHECK --check-prefix=KEEP %s
 
+## Test that keep-section overrides only-keep-debug, even for known sections.
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-strip --only-keep-debug --keep-section=TYPE %t
+# RUN: obj2yaml %t | FileCheck --implicit-check-not=Name --check-prefix=CHECK --check-prefix=KEEPTYPE %s
+
 # CHECK:      Sections:
+# KEEPTYPE:       Type: TYPE
 # CHECK:        - Type: CUSTOM
 # CHECK-NEXT:     Name: .debug_info
 # CHECK:        - Type: CUSTOM
 # CHECK-NEXT:     Name: .debug_line
 # KEEP:           Name: foo
 
 ## Test that remove-section overrides only-keep-debug.
 # RUN: yaml2obj %s -o %t

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

 ## Test the --remove-section flag.
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-objcopy -R producers %t %t2
 # RUN: obj2yaml %t2 | FileCheck --implicit-check-not=producers %s
 ## Check that the producers section has been removed, but not the type section.
 # CHECK: TYPE
 
 ## Requests to remove nonexistent sections are silently ignored.
 # RUN: llvm-objcopy --remove-section=nonexistent=%t.sec %t 2>&1 | count 0
 
+## Remove the type section.
+# RUN: llvm-objcopy -R TYPE %t %t3
+# RUN: obj2yaml %t3 | FileCheck --check-prefix=REMOVETYPE --implicit-check-not=TYPE %s
+## Check that the producers section is still there.
+# REMOVETYPE: producers
+
 --- !WASM
 FileHeader:
   Version: 0x00000001
 Sections:
   - Type: TYPE
     Signatures:
       - Index: 0
         ParamTypes:
           - I32
         ReturnTypes:
           - F32
   - Type: CUSTOM
     Name: producers
     Tools:
       - Name:   clang
         Version: 9.0.0