From d4257740dd2da32306a8068166921f4d716a003e Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Thu, 10 Dec 2020 16:40:08 -0800 Subject: [PATCH] [WebAssembly] Support COMDAT sections in assembly syntax This CL changes the asm syntax for section flags, making them more like ELF (previously "passive" was the only option). Now we also allow "G" to designate COMDAT group sections. In these sections we set the appropriate comdat flag on function symbols, and also avoid auto-creating a new section for them. This also adds asm-based tests for the changes D92691 to go along with the direct-to-object tests. Differential Revision: https://reviews.llvm.org/D92952 This is a reland of rG4564553b8d8a with a fix to the lit pipeline in llvm/test/MC/WebAssembly/comdat.ll --- lib/MC/MCParser/WasmAsmParser.cpp | 80 ++++++++++++++----- lib/MC/MCSectionWasm.cpp | 10 ++- lib/MC/WasmObjectWriter.cpp | 3 + .../AsmParser/WebAssemblyAsmParser.cpp | 17 +++- test/MC/WebAssembly/comdat-sections.ll | 27 +++++-- test/MC/WebAssembly/comdat-sections.s | 50 ++++++++++++ test/MC/WebAssembly/comdat.ll | 20 +++++ 7 files changed, 177 insertions(+), 30 deletions(-) create mode 100644 test/MC/WebAssembly/comdat-sections.s diff --git a/lib/MC/MCParser/WasmAsmParser.cpp b/lib/MC/MCParser/WasmAsmParser.cpp index c8f9807d0c5..0c255ef02d2 100644 --- a/lib/MC/MCParser/WasmAsmParser.cpp +++ b/lib/MC/MCParser/WasmAsmParser.cpp @@ -90,15 +90,40 @@ public: return false; } - bool parseSectionFlags(StringRef FlagStr, bool &Passive) { - SmallVector Flags; - // If there are no flags, keep Flags empty - FlagStr.split(Flags, ",", -1, false); - for (auto &Flag : Flags) { - if (Flag == "passive") + bool parseSectionFlags(StringRef FlagStr, bool &Passive, bool &Group) { + for (char C : FlagStr) { + switch (C) { + case 'p': Passive = true; - else - return error("Expected section flags, instead got: ", Lexer->getTok()); + break; + case 'G': + Group = true; + break; + default: + return Parser->Error(getTok().getLoc(), + StringRef("Unexepcted section flag: ") + FlagStr); + } + } + return false; + } + + bool parseGroup(StringRef &GroupName) { + if (Lexer->isNot(AsmToken::Comma)) + return TokError("expected group name"); + Lex(); + if (Lexer->is(AsmToken::Integer)) { + GroupName = getTok().getString(); + Lex(); + } else if (Parser->parseIdentifier(GroupName)) { + return TokError("invalid group name"); + } + if (Lexer->is(AsmToken::Comma)) { + Lex(); + StringRef Linkage; + if (Parser->parseIdentifier(Linkage)) + return TokError("invalid linkage"); + if (Linkage != "comdat") + return TokError("Linkage must be 'comdat'"); } return false; } @@ -130,27 +155,34 @@ public: if (!Kind.hasValue()) return Parser->Error(Lexer->getLoc(), "unknown section kind: " + Name); - MCSectionWasm *Section = getContext().getWasmSection(Name, Kind.getValue()); // Update section flags if present in this .section directive bool Passive = false; - if (parseSectionFlags(getTok().getStringContents(), Passive)) + bool Group = false; + if (parseSectionFlags(getTok().getStringContents(), Passive, Group)) return true; - if (Passive) { - if (!Section->isWasmData()) - return Parser->Error(getTok().getLoc(), - "Only data sections can be passive"); - Section->setPassive(); - } - Lex(); - if (expect(AsmToken::Comma, ",") || expect(AsmToken::At, "@") || - expect(AsmToken::EndOfStatement, "eol")) + if (expect(AsmToken::Comma, ",") || expect(AsmToken::At, "@")) return true; - auto WS = getContext().getWasmSection(Name, Kind.getValue()); + StringRef GroupName; + if (Group && parseGroup(GroupName)) + return true; + + if (expect(AsmToken::EndOfStatement, "eol")) + return true; + + // TODO: Parse UniqueID + MCSectionWasm *WS = getContext().getWasmSection( + Name, Kind.getValue(), GroupName, MCContext::GenericSectionID); + if (Passive) { + if (!WS->isWasmData()) + return Parser->Error(getTok().getLoc(), + "Only data sections can be passive"); + WS->setPassive(); + } getStreamer().SwitchSection(WS); return false; } @@ -189,9 +221,13 @@ public: Lexer->is(AsmToken::Identifier))) return error("Expected label,@type declaration, got: ", Lexer->getTok()); auto TypeName = Lexer->getTok().getString(); - if (TypeName == "function") + if (TypeName == "function") { WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION); - else if (TypeName == "global") + auto *Current = + cast(getStreamer().getCurrentSection().first); + if (Current->getGroup()) + WasmSym->setComdat(true); + } else if (TypeName == "global") WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL); else if (TypeName == "object") WasmSym->setType(wasm::WASM_SYMBOL_TYPE_DATA); diff --git a/lib/MC/MCSectionWasm.cpp b/lib/MC/MCSectionWasm.cpp index 27ed51802a2..81dc4329be6 100644 --- a/lib/MC/MCSectionWasm.cpp +++ b/lib/MC/MCSectionWasm.cpp @@ -64,7 +64,9 @@ void MCSectionWasm::PrintSwitchToSection(const MCAsmInfo &MAI, const Triple &T, OS << ",\""; if (IsPassive) - OS << "passive"; + OS << "p"; + if (Group) + OS << "G"; OS << '"'; @@ -78,6 +80,12 @@ void MCSectionWasm::PrintSwitchToSection(const MCAsmInfo &MAI, const Triple &T, // TODO: Print section type. + if (Group) { + OS << ","; + printName(OS, Group->getName()); + OS << ",comdat"; + } + if (isUnique()) OS << ",unique," << UniqueID; diff --git a/lib/MC/WasmObjectWriter.cpp b/lib/MC/WasmObjectWriter.cpp index 77df4acfe11..c8d2e5dbdbd 100644 --- a/lib/MC/WasmObjectWriter.cpp +++ b/lib/MC/WasmObjectWriter.cpp @@ -1367,6 +1367,9 @@ uint64_t WasmObjectWriter::writeOneObject(MCAssembler &Asm, if (Mode == DwoMode::DwoOnly && !isDwoSection(Sec)) continue; + LLVM_DEBUG(dbgs() << "Processing Section " << SectionName << " group " + << Section.getGroup() << "\n";); + // .init_array sections are handled specially elsewhere. if (SectionName.startswith(".init_array")) continue; diff --git a/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp b/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp index 7426d87ccbc..55f79bafe41 100644 --- a/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp +++ b/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp @@ -971,12 +971,27 @@ public: auto SymName = Symbol->getName(); if (SymName.startswith(".L")) return; // Local Symbol. + // Only create a new text section if we're already in one. + // TODO: If the user explicitly creates a new function section, we ignore + // its name when we create this one. It would be nice to honor their + // choice, while still ensuring that we create one if they forget. + // (that requires coordination with WasmAsmParser::parseSectionDirective) auto CWS = cast(getStreamer().getCurrentSection().first); if (!CWS || !CWS->getKind().isText()) return; auto SecName = ".text." + SymName; - auto WS = getContext().getWasmSection(SecName, SectionKind::getText()); + + auto *Group = CWS->getGroup(); + // If the current section is a COMDAT, also set the flag on the symbol. + // TODO: Currently the only place that the symbols' comdat flag matters is + // for importing comdat functions. But there's no way to specify that in + // assembly currently. + if (Group) + cast(Symbol)->setComdat(true); + auto *WS = + getContext().getWasmSection(SecName, SectionKind::getText(), Group, + MCContext::GenericSectionID, nullptr); getStreamer().SwitchSection(WS); // Also generate DWARF for this section if requested. if (getContext().getGenDwarfForAssembly()) diff --git a/test/MC/WebAssembly/comdat-sections.ll b/test/MC/WebAssembly/comdat-sections.ll index 75381ff6f90..bb8421829f0 100644 --- a/test/MC/WebAssembly/comdat-sections.ll +++ b/test/MC/WebAssembly/comdat-sections.ll @@ -1,13 +1,28 @@ ; RUN: llc -dwarf-version=4 -generate-type-units \ ; RUN: -filetype=obj -O0 -mtriple=wasm32-unknown-unknown < %s \ -; RUN: | obj2yaml | FileCheck %s +; RUN: | obj2yaml | FileCheck --check-prefix=OBJ %s + +; RUN: llc -dwarf-version=4 -generate-type-units \ +; RUN: -filetype=asm -O0 -mtriple=wasm32-unknown-unknown < %s \ +; RUN: | FileCheck --check-prefix=ASM %s -; CHECK: Comdats: -; CHECK: - Name: '4721183873463917179' -; CHECK: Entries: -; CHECK: - Kind: SECTION -; CHECK: Index: 3 +; OBJ: Comdats: +; OBJ-NEXT: - Name: '4721183873463917179' +; OBJ-NEXT: Entries: +; OBJ-NEXT: - Kind: SECTION +; OBJ-NEXT: Index: 3 + + +; ASM: .section .debug_types,"G",@,4721183873463917179,comdat +; Here we are not trying to verify all of the debug info; just enough to ensure +; that the section contains a type unit for a type with matching signature +; ASM-NEXT: .int32 .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit +; ASM-NEXT: .Ldebug_info_start0: +; ASM-NEXT: .int16 4 # DWARF version number +; ASM-NEXT: .int32 .debug_abbrev0 # Offset Into Abbrev. Section +; ASM-NEXT: .int8 4 # Address Size (in bytes) +; ASM-NEXT: .int64 4721183873463917179 # Type Signature ; ModuleID = 't.cpp' source_filename = "t.cpp" diff --git a/test/MC/WebAssembly/comdat-sections.s b/test/MC/WebAssembly/comdat-sections.s new file mode 100644 index 00000000000..291e5c6f8de --- /dev/null +++ b/test/MC/WebAssembly/comdat-sections.s @@ -0,0 +1,50 @@ +# RUN: llvm-mc -triple=wasm32 -filetype=obj %s -o - | obj2yaml | FileCheck %s + + .section .text.foo,"G",@,abc123,comdat + .globl foo + .type foo,@function +foo: + .functype foo () -> () + return + end_function + + .globl bar +bar: + .functype bar () -> () + return + end_function + + .section .debug_foo,"G",@,abc123,comdat + .int32 42 + .section .debug_foo,"G",@,duplicate,comdat + .int64 234 + +# Check that there are 2 identically-named custom sections, with the desired +# contents +# CHECK: - Type: CUSTOM +# CHECK-NEXT: Name: .debug_foo +# CHECK-NEXT: Payload: 2A000000 +# CHECK-NEXT: - Type: CUSTOM +# CHECK-NEXT: Name: .debug_foo +# CHECK-NEXT: Payload: EA00000000000000 + +# And check that they are in 2 different comdat groups +# CHECK-NEXT:- Type: CUSTOM +# CHECK-NEXT: Name: linking +# CHECK-NEXT: Version: 2 +# CHECK: Comdats: +# CHECK-NEXT: - Name: abc123 +# CHECK-NEXT: Entries: +# CHECK-NEXT: - Kind: FUNCTION +# CHECK-NEXT: Index: 0 + +# If the user forgets to create a new section for a function, one is created for +# them by the assembler. Check that it is also in the same group. +# CHECK-NEXT: - Kind: FUNCTION +# CHECK-NEXT: Index: 1 +# CHECK-NEXT: - Kind: SECTION +# CHECK-NEXT: Index: 4 +# CHECK-NEXT: - Name: duplicate +# CHECK-NEXT: Entries: +# CHECK-NEXT: - Kind: SECTION +# CHECK-NEXT: Index: 5 diff --git a/test/MC/WebAssembly/comdat.ll b/test/MC/WebAssembly/comdat.ll index f7809e859ef..5af940d1ad5 100644 --- a/test/MC/WebAssembly/comdat.ll +++ b/test/MC/WebAssembly/comdat.ll @@ -1,4 +1,8 @@ ; RUN: llc -filetype=obj %s -o - | obj2yaml | FileCheck %s +; RUN: llc -filetype=asm %s -asm-verbose=false -o - | FileCheck --check-prefix=ASM %s +; RUN: llc -filetype=asm %s -o - | llvm-mc -triple=wasm32 -filetype=obj -o - | obj2yaml | FileCheck %s +; These RUN lines verify the ll direct-to-object path, the ll->asm path, and the +; object output via asm. target triple = "wasm32-unknown-unknown" @@ -116,3 +120,19 @@ define linkonce_odr i32 @sharedFn() #1 comdat($sharedComdat) { ; CHECK-NEXT: - Kind: DATA ; CHECK-NEXT: Index: 0 ; CHECK-NEXT: ... + + +; ASM: .section .text.basicInlineFn,"G",@,basicInlineFn,comdat +; ASM-NEXT: .weak basicInlineFn +; ASM-NEXT: .type basicInlineFn,@function +; ASM-NEXT: basicInlineFn: + +; ASM: .section .text.sharedFn,"G",@,sharedComdat,comdat +; ASM-NEXT: .weak sharedFn +; ASM-NEXT: .type sharedFn,@function +; ASM-NEXT: sharedFn: + +; ASM: .type constantData,@object +; ASM-NEXT: .section .rodata.constantData,"G",@,sharedComdat,comdat +; ASM-NEXT: .weak constantData +; ASM-NEXT: constantData: