[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

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

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

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

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 ↗

(On Diff #433102)

Just only empty line here?

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

13 ↗

(On Diff #433102)

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 ↗

(On Diff #433102)

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 ↗

(On Diff #433150)

[aheejin]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

30 ↗

(On Diff #433150)

Maybe create WASM_LAST_SEC or something?

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

17 ↗

(On Diff #433150)

[dschuff]

llvm/lib/ObjCopy/wasm/WasmReader.cpp

30 ↗

(On Diff #433150)

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 ↗

(On Diff #433150)

Not something important, but gentle ping?

[dschuff]

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

17 ↗

(On Diff #433150)

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

[aheejin]

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

17 ↗

(On Diff #433150)

# CHECK-NOT:    - Type: TYPE

is still missing. Not important though.

[dschuff]

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

17 ↗

(On Diff #433150)

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 ↗

(On Diff #433150)

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 ↗

(On Diff #433150)

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.

lld/wasm/OutputSections.cpp

 // Returns a string, e.g. "FUNCTION(.text)".
 std::string toString(const wasm::OutputSection &sec) {
   if (!sec.name.empty())
     return (sec.getSectionName() + "(" + sec.name + ")").str();
   return std::string(sec.getSectionName());
 }
 
 namespace wasm {
-static StringRef sectionTypeToString(uint32_t sectionType) {
-  switch (sectionType) {
-  case WASM_SEC_CUSTOM:
-    return "CUSTOM";
-  case WASM_SEC_TYPE:
-    return "TYPE";
-  case WASM_SEC_IMPORT:
-    return "IMPORT";
-  case WASM_SEC_FUNCTION:
-    return "FUNCTION";
-  case WASM_SEC_TABLE:
-    return "TABLE";
-  case WASM_SEC_MEMORY:
-    return "MEMORY";
-  case WASM_SEC_GLOBAL:
-    return "GLOBAL";
-  case WASM_SEC_TAG:
-    return "TAG";
-  case WASM_SEC_EXPORT:
-    return "EXPORT";
-  case WASM_SEC_START:
-    return "START";
-  case WASM_SEC_ELEM:
-    return "ELEM";
-  case WASM_SEC_CODE:
-    return "CODE";
-  case WASM_SEC_DATA:
-    return "DATA";
-  case WASM_SEC_DATACOUNT:
-    return "DATACOUNT";
-  default:
-    fatal("invalid section type");
-  }
-}
-
 StringRef OutputSection::getSectionName() const {
   return sectionTypeToString(type);
 }
 
 void OutputSection::createHeader(size_t bodySize) {
   raw_string_ostream os(header);
   debugWrite(os.tell(), "section type [" + getSectionName() + "]");
   encodeULEB128(type, os);

llvm/lib/BinaryFormat/Wasm.cpp

 #include "llvm/BinaryFormat/WasmRelocs.def"
 #undef WASM_RELOC
   default:
     llvm_unreachable("unknown reloc type");
   }
 }
 
 llvm::StringRef llvm::wasm::sectionTypeToString(uint32_t Type) {
+#define ECase(X)                                                               \
+  case wasm::WASM_SEC_##X:                                                     \
+    return #X;
   switch (Type) {
-  case WASM_SEC_CUSTOM:
-    return "CUSTOM";
-  case WASM_SEC_TYPE:
-    return "TYPE";
-  case WASM_SEC_IMPORT:
-    return "IMPORT";
-  case WASM_SEC_FUNCTION:
-    return "FUNCTION";
-  case WASM_SEC_TABLE:
-    return "TABLE";
-  case WASM_SEC_MEMORY:
-    return "MEMORY";
-  case WASM_SEC_GLOBAL:
-    return "GLBOAL";
-  case WASM_SEC_EXPORT:
-    return "EXPORT";
-  case WASM_SEC_START:
-    return "START";
-  case WASM_SEC_ELEM:
-    return "ELEM";
-  case WASM_SEC_CODE:
-    return "CODE";
-  case WASM_SEC_DATA:
-    return "DATA";
-  case WASM_SEC_DATACOUNT:
-    return "DATACOUNT";
-  case WASM_SEC_TAG:
-    return "TAG";
+    ECase(CUSTOM);
+    ECase(TYPE);
+    ECase(IMPORT);
+    ECase(FUNCTION);
+    ECase(TABLE);
+    ECase(MEMORY);
+    ECase(GLOBAL);
+    ECase(EXPORT);
+    ECase(START);
+    ECase(ELEM);
+    ECase(CODE);
+    ECase(DATA);
+    ECase(DATACOUNT);
+    ECase(TAG);
   default:
     llvm_unreachable("unknown section type");
   }
+#undef ECase
 }
 
 bool llvm::wasm::relocTypeHasAddend(uint32_t Type) {
   switch (Type) {
   case R_WASM_MEMORY_ADDR_LEB:
   case R_WASM_MEMORY_ADDR_LEB64:
   case R_WASM_MEMORY_ADDR_SLEB:
   case R_WASM_MEMORY_ADDR_SLEB64:
   case R_WASM_MEMORY_ADDR_REL_SLEB:
   case R_WASM_MEMORY_ADDR_REL_SLEB64:
   case R_WASM_MEMORY_ADDR_I32:
   case R_WASM_MEMORY_ADDR_I64:
   case R_WASM_MEMORY_ADDR_TLS_SLEB:
   case R_WASM_MEMORY_ADDR_TLS_SLEB64:
   case R_WASM_FUNCTION_OFFSET_I32:
   case R_WASM_FUNCTION_OFFSET_I64:
   case R_WASM_SECTION_OFFSET_I32:
   case R_WASM_MEMORY_ADDR_LOCREL_I32:
     return true;
   default:
     return false;
   }
 }

llvm/lib/Object/WasmObjectFile.cpp

default:
     llvm_unreachable("unknown WasmSymbol::SymbolType");
   }
 }
 
 void WasmObjectFile::moveSectionNext(DataRefImpl &Sec) const { Sec.d.a++; }
 
 Expected<StringRef> WasmObjectFile::getSectionName(DataRefImpl Sec) const {
   const WasmSection &S = Sections[Sec.d.a];
-#define ECase(X)                                                               \
-  case wasm::WASM_SEC_##X:                                                     \
-    return #X;
-  switch (S.Type) {
-    ECase(TYPE);
-    ECase(IMPORT);
-    ECase(FUNCTION);
-    ECase(TABLE);
-    ECase(MEMORY);
-    ECase(GLOBAL);
-    ECase(TAG);
-    ECase(EXPORT);
-    ECase(START);
-    ECase(ELEM);
-    ECase(CODE);
-    ECase(DATA);
-    ECase(DATACOUNT);
-  case wasm::WASM_SEC_CUSTOM:
+  if (S.Type == wasm::WASM_SEC_CUSTOM)
     return S.Name;
-  default:
+  if (S.Type > wasm::WASM_SEC_TAG)
     return createStringError(object_error::invalid_section_index, "");
-  }
-#undef ECase
+  return wasm::sectionTypeToString(S.Type);
 }
 
 uint64_t WasmObjectFile::getSectionAddress(DataRefImpl Sec) const { return 0; }
 
 uint64_t WasmObjectFile::getSectionIndex(DataRefImpl Sec) const {
   return Sec.d.a;
 }