[WebAssembly] ensure .functype applies to right label in assembler

[aardappel]

Summary

We used to require .functype immediately follows the label it sets the type of, but not all Clang output follows this rule.

Now we simply allow it on any symbol, but only assume its a function start for a defined symbol, which is simpler and more general.

Fixes (part of) https://bugs.llvm.org/show_bug.cgi?id=49036

[aardappel]

Fixes (part of) https://bugs.llvm.org/show_bug.cgi?id=49036

[sbc100]

lld/test/wasm/command-exports.s

44 ↗

(On Diff #321833)

This looks a little worrying since I imagine what we are looking at here output from a compiler pass.

Can you check that WebAssemblyLowerGlobalDtors doesn't generate functions with local names like this? I think you can just build anything with static C++ ctor/dtor to see it in action.

[aardappel]

lld/test/wasm/command-exports.s

44 ↗

(On Diff #321833)

Good catch, it generates functions starting with register_call_dtors with Function::PrivateLinkage, which I guess at an asm level turns into .L labels.

[aardappel]

simplified and generalized .functype handing

[sbc100]

Wow that is way better. Assuming all the tests pass LGTM!

[sbc100]

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

227

This comment seems like it might still be relevant to the remaining LastFunctionLabel?

[aardappel]

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

227

It doesn't, because we don't require .functype to follow labels at all anymore.

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

class WebAssemblyAsmParser final : public MCTargetAsmParser {
   // Order of labels, directives and instructions in a .s file have no
   // syntactical enforcement. This class is a callback from the actual parser,
   // and yet we have to be feeding data to the streamer in a very particular
   // order to ensure a correct binary encoding that matches the regular backend
   // (the streamer does not enforce this). This "state machine" enum helps
   // guarantee that correct order.
   enum ParserState {
     FileStart,
-    Label,
     FunctionStart,
     FunctionLocals,
     Instructions,
     EndFunction,
     DataSection,
   } CurrentState = FileStart;
 
   // For ensuring blocks are properly nested.
   enum NestingType {
     Function,
     Block,
     Loop,
     Try,
     If,
     Else,
     Undefined,
   };
   std::vector<NestingType> NestingStack;
 
-  // We track this to see if a .functype following a label is the same,
-  // as this is how we recognize the start of a function.
-  MCSymbol *LastLabel = nullptr;
   MCSymbol *LastFunctionLabel = nullptr;
 
 public:
   WebAssemblyAsmParser(const MCSubtargetInfo &STI, MCAsmParser &Parser,
                        const MCInstrInfo &MII, const MCTargetOptions &Options)
       : MCTargetAsmParser(Options, STI, MII), Parser(Parser),
         Lexer(Parser.getLexer()) {
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
bool ParseInstruction(ParseInstructionInfo & /*Info*/, StringRef Name,
     if (ExpectBlockType && Operands.size() == 1) {
       // Support blocks with no operands as default to void.
       addBlockTypeOperand(Operands, NameLoc, WebAssembly::BlockType::Void);
     }
     Parser.Lex();
     return false;
   }
 
-  void onLabelParsed(MCSymbol *Symbol) override {
-    LastLabel = Symbol;
-    CurrentState = Label;
-  }
-
   bool parseSignature(wasm::WasmSignature *Signature) {
     if (expect(AsmToken::LParen, "("))
       return true;
     if (parseRegTypeList(Signature->Params))
       return true;
     if (expect(AsmToken::RParen, ")"))
       return true;
     if (expect(AsmToken::MinusGreater, "->"))
if (DirectiveID.getString() == ".functype") {
       // WebAssemblyAsmPrinter::EmitFunctionBodyStart.
       // TODO: would be good to factor this into a common function, but the
       // assembler and backend really don't share any common code, and this code
       // parses the locals separately.
       auto SymName = expectIdent();
       if (SymName.empty())
         return true;
       auto WasmSym = cast<MCSymbolWasm>(Ctx.getOrCreateSymbol(SymName));
-      if (CurrentState == Label && WasmSym == LastLabel) {
+      if (WasmSym->isDefined()) {
         // This .functype indicates a start of a function.
         if (ensureEmptyNestingStack())
           return true;
         CurrentState = FunctionStart;
-        LastFunctionLabel = LastLabel;
+        LastFunctionLabel = WasmSym;
         push(Function);
       }
       auto Signature = std::make_unique<wasm::WasmSignature>();
       if (parseSignature(Signature.get()))
         return true;
       WasmSym->setSignature(Signature.get());
       addSignature(std::move(Signature));
       WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
if (DirectiveID.getString() == ".eventtype") {
       WasmSym->setType(wasm::WASM_SYMBOL_TYPE_EVENT);
       TOut.emitEventType(WasmSym);
       // TODO: backend also calls TOut.emitIndIdx, but that is not implemented.
       return expect(AsmToken::EndOfStatement, "EOL");
     }
 
     if (DirectiveID.getString() == ".local") {
       if (CurrentState != FunctionStart)
-        return error(".local directive should follow the start of a function",
+        return error(".local directive should follow the start of a function: ",
                      Lexer.getTok());
       SmallVector<wasm::ValType, 4> Locals;
       if (parseRegTypeList(Locals))
         return true;
       TOut.emitLocal(Locals);
       CurrentState = FunctionLocals;
       return expect(AsmToken::EndOfStatement, "EOL");
     }

llvm/test/MC/WebAssembly/basic-assembly.s

 # RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+atomics,+unimplemented-simd128,+nontrapping-fptoint,+exception-handling < %s | FileCheck %s
 # Check that it converts to .o without errors, but don't check any output:
 # RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj -mattr=+atomics,+unimplemented-simd128,+nontrapping-fptoint,+exception-handling -o %t.o < %s
 
 
 empty_func:
     .functype empty_func () -> ()
     end_function
 
 test0:
+# local labels can appear between label and its .functype.
+.Ltest0begin:
     # Test all types:
     .functype   test0 (i32, i64) -> (i32)
     .eventtype  __cpp_exception i32
     .local      f32, f64, v128, v128
     # Explicit getlocal/setlocal:
     local.get   2
     local.set   2
     # Immediates:
.L.str:
     .section    .init_array.42,"",@
     .p2align    2
     .int32      test0
 
     .ident      "clang version 9.0.0 (trunk 364502) (llvm/trunk 364571)"
     .globaltype __stack_pointer, i32
 
 .tabletype empty_eref_table, externref
-empty_eref_table:       
+empty_eref_table:
 
 .tabletype empty_fref_table, funcref
-empty_fref_table:       
+empty_fref_table:
 
-        
 # CHECK:           .text
 # CHECK-LABEL: empty_func:
 # CHECK-NEXT:      .functype  empty_func () -> ()
 # CHECK-NEXT:      end_function
 # CHECK-LABEL: test0:
+# CHECK-NEXT:  .Ltest0begin:
 # CHECK-NEXT:      .functype   test0 (i32, i64) -> (i32)
 # CHECK-NEXT:      .eventtype  __cpp_exception i32
 # CHECK-NEXT:      .local      f32, f64
 # CHECK-NEXT:      local.get   2
 # CHECK-NEXT:      local.set   2
 # CHECK-NEXT:      i32.const   -1
 # CHECK-NEXT:      f64.const   0x1.999999999999ap1
 # CHECK-NEXT:      f32.const   -0x1p0

 # CHECK:           .section    .init_array.42,"",@
 # CHECK-NEXT:      .p2align    2
 # CHECK-NEXT:      .int32      test0
 
 # CHECK:           .globaltype __stack_pointer, i32
 
 # CHECK:           .tabletype empty_eref_table, externref
 # CHECK-NEXT: empty_eref_table:
-        
 # CHECK:           .tabletype empty_fref_table, funcref
 # CHECK-NEXT: empty_fref_table: