[WebAssembly] Support COMDAT sections in assembly syntax

[dschuff]

Summary

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

[dschuff]

Fix up

[dschuff]

This is ready to review now.

[dschuff]

Simplify WebAssemblyAsmParser, add test for its behavior, add TODO

[dschuff]

fix rebase

[dschuff]

OK, added a check for the behavior when the .section directive is missing for a function, and a TODO for the existing problem of honoring the user's specified section name.

[sbc100]

Great!

llvm/lib/MC/MCParser/WasmAsmParser.cpp

172

Combine into single condition?

llvm/lib/MC/WasmObjectWriter.cpp

1370

Did you mean to leave this in?

llvm/test/MC/WebAssembly/comdat-asm.ll

28

I think we normally run with -asm-verbose=false to avoid these extra comments.

llvm/test/MC/WebAssembly/comdat-sections.s

5

I normally drop the .type foo,@function this its not needed.

23

Just one empty line here?

llvm/test/MC/WebAssembly/comdat.ll

5

Do we still need comdat-asm.ll given that this test runs in both modes?

[dschuff]

• review

[dschuff]

• Simplify; add test for forgetting to specify sectoin
• review
• remove comdat-asm.ll, rebase

[sbc100]

lgtm

( i didn't see any response the the two comments, one regarding the extra debugging output and the other regarding the include the .type foo,@function lines in test code).

[dschuff]

oops, forgot to publish my comments.

llvm/lib/MC/WasmObjectWriter.cpp

1370

Yeah I figured it was useful to go along with the other debug prints behind LLVM_DEBUG in this file.

llvm/test/MC/WebAssembly/comdat-sections.s

5

Ah, you've actually hit on something interesting here. Currently the logic that sets the comdat flag on the function symbol is in WasmAsmParser::parseDirectiveType(), so it doesn't run if we leave out .type. That function also sets the symbol type. I don't see any logic anywhere else in WasmAsmParser or WebAssemblyAsmParser that sets the symbol type, but I guess it just defaults to function?

I tried it without the .type, and found that the tests all still worked! I found that that only the group flag on the section matters for creating the linking section. But other code still checks the comdat flag on the symbol, e.g. for creating imports. I updated the code for WebAssemblyAsmParser in this CL and I think it should work now, (i.e. it keeps the section group and symbol comdat proerties in sync). But I'm not sure how to test it. How would you specify an imported comdat function in assembly? Currently the only way to specify comdats is by using .section but that doesn't make sense for an import.

For now I put the .type there for foo, but left it off .bar, to exercise both codepaths. We can figure out the imported-comdat problem in a future CL.

llvm/test/MC/WebAssembly/comdat.ll

5

oh oops yes, I forgot I created comdat-asm.ll and then merged it in to here.

[sbc100]

IIUC comdat is a propoerty shared by a group of a definitions (not declarations) so I don't see who an import could be part of a comdat group.

llvm/lib/MC/MCParser/WasmAsmParser.cpp

bool expect(AsmToken::TokenKind Kind, const char *KindName) {
     return false;
   }
 
   bool parseSectionDirectiveText(StringRef, SMLoc) {
     // FIXME: .text currently no-op.
     return false;
   }
 
-  bool parseSectionFlags(StringRef FlagStr, bool &Passive) {
-    SmallVector<StringRef, 2> 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;
   }
 
   bool parseSectionDirective(StringRef, SMLoc) {
     StringRef Name;
     if (Parser->parseIdentifier(Name))
       return TokError("expected identifier in directive");
auto Kind = StringSwitch<Optional<SectionKind>>(Name)
                     // See use of .init_array in WasmObjectWriter and
                     // TargetLoweringObjectFileWasm
                     .StartsWith(".init_array", SectionKind::getData())
                     .StartsWith(".debug_", SectionKind::getMetadata())
                     .Default(Optional<SectionKind>());
     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;
+
+    StringRef GroupName;
+    if (Group) {
+      if (parseGroup(GroupName))
+        return true;
+    }
+
+    if (expect(AsmToken::EndOfStatement, "eol"))
       return true;
 
-    auto WS = getContext().getWasmSection(Name, Kind.getValue());
+    // 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;
   }
 
   // TODO: This function is almost the same as ELFAsmParser::ParseDirectiveSize
   // so maybe could be shared somehow.
   bool parseDirectiveSize(StringRef, SMLoc) {
     StringRef Name;
bool parseDirectiveType(StringRef, SMLoc) {
     auto WasmSym = cast<MCSymbolWasm>(
                      getStreamer().getContext().getOrCreateSymbol(
                        Lexer->getTok().getString()));
     Lex();
     if (!(isNext(AsmToken::Comma) && isNext(AsmToken::At) &&
           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<MCSectionWasm>(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);
     else
       return error("Unknown WASM symbol type: ", Lexer->getTok());
     Lex();
     return expect(AsmToken::EndOfStatement, "EOL");
   }

llvm/lib/MC/MCSectionWasm.cpp

if (shouldOmitSectionDirective(getName(), MAI)) {
     return;
   }
 
   OS << "\t.section\t";
   printName(OS, getName());
   OS << ",\"";
 
   if (IsPassive)
-    OS << "passive";
+    OS << "p";
+  if (Group)
+    OS << "G";
 
   OS << '"';
 
   OS << ',';
 
   // If comment string is '@', e.g. as on ARM - use '%' instead
   if (MAI.getCommentString()[0] == '@')
     OS << '%';
   else
     OS << '@';
 
   // TODO: Print section type.
 
+  if (Group) {
+    OS << ",";
+    printName(OS, Group->getName());
+    OS << ",comdat";
+  }
+
   if (isUnique())
     OS << ",unique," << UniqueID;
 
   OS << '\n';
 
   if (Subsection) {
     OS << "\t.subsection\t";
     Subsection->print(OS, &MAI);
     OS << '\n';
   }
 }
 
 bool MCSectionWasm::UseCodeAlign() const { return false; }
 
 bool MCSectionWasm::isVirtualSection() const { return false; }

llvm/lib/MC/WasmObjectWriter.cpp

for (MCSection &Sec : Asm) {
     auto &Section = static_cast<MCSectionWasm &>(Sec);
     StringRef SectionName = Section.getName();
 
     if (Mode == DwoMode::NonDwoOnly && isDwoSection(Sec))
       continue;
     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;
 
     // Code is handled separately
     if (Section.getKind().isText())
       continue;
 

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

#include "WebAssemblyGenAsmMatcher.inc"
 
   void doBeforeLabelEmit(MCSymbol *Symbol) override {
     // Start a new section for the next function automatically, since our
     // object writer expects each function to have its own section. This way
     // The user can't forget this "convention".
     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<MCSectionWasm>(getStreamer().getCurrentSection().first);
     if (!CWS || !CWS->getKind().isText())
       return;
     auto SecName = ".text." + SymName;
-    auto WS = getContext().getWasmSection(SecName, SectionKind::getText());
+
+    auto WS = getContext().getWasmSection(SecName, SectionKind::getText(),
+                                          CWS->getGroup(),
+                                          MCContext::GenericSectionID, nullptr);
     getStreamer().SwitchSection(WS);
     // Also generate DWARF for this section if requested.
     if (getContext().getGenDwarfForAssembly())
       getContext().addGenDwarfSection(WS);
   }
 
   void onEndOfFunction() {
     // Automatically output a .size directive, so it becomes optional for the

llvm/test/MC/WebAssembly/comdat-asm.ll

+; RUN: llc -filetype=asm %s -o -  | FileCheck %s
+
+target triple = "wasm32-unknown-unknown"
+
+; Import a function just so we can check the index arithmetic for
+; WASM_COMDAT_FUNCTION entries is performed correctly
+declare i32 @funcImport()
+define i32 @callImport() {
+entry:
+  %call = call i32 @funcImport()
+  ret i32 %call
+}
+
+; Function in its own COMDAT
+$basicInlineFn = comdat any
+define linkonce_odr i32 @basicInlineFn() #1 comdat {
+  ret i32 0
+}
+
+; Global, data, and function in same COMDAT
+$sharedComdat = comdat any
+@constantData = weak_odr constant [3 x i8] c"abc", comdat($sharedComdat)
+define linkonce_odr i32 @sharedFn() #1 comdat($sharedComdat) {
+  ret i32 0
+}
+
+; CHECK:        .section        .text.basicInlineFn,"G",@,basicInlineFn,comdat
+; CHECK-NEXT:        .weak   basicInlineFn                   # -- Begin function basicInlineFn
+; CHECK-NEXT:        .type   basicInlineFn,@function
+; CHECK-NEXT: basicInlineFn:                          # @basicInlineFn
+
+; CHECK:        .section        .text.sharedFn,"G",@,sharedComdat,comdat
+; CHECK-NEXT:        .weak   sharedFn                        # -- Begin function sharedFn
+; CHECK-NEXT:        .type   sharedFn,@function
+; CHECK-NEXT: sharedFn:                               # @sharedFn
+
+; CHECK:        .type   constantData,@object            # @constantData
+; CHECK-NEXT:        .section        .rodata.constantData,"G",@,sharedComdat,comdat
+; CHECK-NEXT:        .weak   constantData
+; CHECK-NEXT: constantData:

llvm/test/MC/WebAssembly/comdat-sections.ll

 ; 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"
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown-wasm"
 
 %struct.S = type { i32 }
 

llvm/test/MC/WebAssembly/comdat-sections.s

+# 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
+        .type bar,@function
+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

llvm/test/MC/WebAssembly/comdat.ll

 ; RUN: llc -filetype=obj %s -o - | obj2yaml | FileCheck %s
+; RUN: llc -filetype=asm %s -o -  | FileCheck --check-prefix=ASM %s
+; RUN: llc -filetype=asm %s -o %t.s | llvm-mc -triple=wasm32 -filetype=obj %t.s -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"
 
 ; Import a function just so we can check the index arithmetic for
 ; WASM_COMDAT_FUNCTION entries is performed correctly
 declare i32 @funcImport()
 define i32 @callImport() {
 entry:

 ; CHECK-NEXT:            Index:           2
 ; CHECK-NEXT:        Name:            sharedComdat
 ; CHECK-NEXT:        Entries:
 ; CHECK-NEXT:          - Kind:            FUNCTION
 ; CHECK-NEXT:            Index:           3
 ; CHECK-NEXT:          - Kind:            DATA
 ; CHECK-NEXT:            Index:           0
 ; CHECK-NEXT: ...
+
+
+; ASM:        .section        .text.basicInlineFn,"G",@,basicInlineFn,comdat
+; ASM-NEXT:        .weak   basicInlineFn                   # -- Begin function basicInlineFn
+; ASM-NEXT:        .type   basicInlineFn,@function
+; ASM-NEXT: basicInlineFn:                          # @basicInlineFn
+
+; ASM:        .section        .text.sharedFn,"G",@,sharedComdat,comdat
+; ASM-NEXT:        .weak   sharedFn                        # -- Begin function sharedFn
+; ASM-NEXT:        .type   sharedFn,@function
+; ASM-NEXT: sharedFn:                               # @sharedFn
+
+; ASM:        .type   constantData,@object            # @constantData
+; ASM-NEXT:        .section        .rodata.constantData,"G",@,sharedComdat,comdat
+; ASM-NEXT:        .weak   constantData
+; ASM-NEXT: constantData: