[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

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

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

218

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

[aardappel]

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

218

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

lld/test/wasm/command-exports.s

some_dtor:
 
   .hidden __cxa_atexit
   .globl  __cxa_atexit
 __cxa_atexit:
   .functype  __cxa_atexit (i32, i32, i32) -> (i32)
   i32.const 0
   end_function
 
-  .section  .text..Lcall_dtors.1,"",@
-.Lcall_dtors.1:
-  .functype  .Lcall_dtors.1 (i32) -> ()
+  .section  .text.call_dtors.1,"",@
+call_dtors.1:
+  .functype call_dtors.1 (i32) -> ()
   call  some_dtor
   end_function
 
-  .section  .text..Lregister_call_dtors.1,"",@
-.Lregister_call_dtors.1:
-  .functype .Lregister_call_dtors.1 () -> ()
+  .section  .text.register_call_dtors.1,"",@
+register_call_dtors.1:
+  .functype register_call_dtors.1 () -> ()
   block
-  i32.const .Lcall_dtors.1
+  i32.const call_dtors.1
   i32.const 0
   i32.const 0
   call  __cxa_atexit
   i32.eqz
   br_if     0
   unreachable
 .LBB6_2:
   end_block
   end_function
 
   .section  .init_array.1,"",@
   .p2align  2
   .int32  some_ctor
-  .int32  .Lregister_call_dtors.1
+  .int32  register_call_dtors.1
   .export_name  foo_i32, foo_i32
   .export_name  foo_f64, foo_f64
 
 # CHECK:       - Type:            EXPORT
 # CHECK-NEXT:    Exports:
 # CHECK-NEXT:      - Name:            memory
 # CHECK-NEXT:        Kind:            MEMORY
 # CHECK-NEXT:        Index:           0

 # CHECK-NEXT:        Name:            foo_f64
 # CHECK-NEXT:      - Index:           3
 # CHECK-NEXT:        Name:            some_ctor
 # CHECK-NEXT:      - Index:           4
 # CHECK-NEXT:        Name:            some_dtor
 # CHECK-NEXT:      - Index:           5
 # CHECK-NEXT:        Name:            __cxa_atexit
 # CHECK-NEXT:      - Index:           6
-# CHECK-NEXT:        Name:            .Lcall_dtors.1
+# CHECK-NEXT:        Name:            call_dtors.1
 # CHECK-NEXT:      - Index:           7
-# CHECK-NEXT:        Name:            .Lregister_call_dtors.1
+# CHECK-NEXT:        Name:            register_call_dtors.1
 # CHECK-NEXT:      - Index:           8
 # CHECK-NEXT:        Name:            foo_i32.command_export
 # CHECK-NEXT:      - Index:           9
 # CHECK-NEXT:        Name:            foo_f64.command_export

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

enum NestingType {
     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 *LastNonLocalLabel = 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()));
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;
+    if (!Symbol->getName().startswith(".L"))
+      LastNonLocalLabel = Symbol;
   }
 
   bool parseSignature(wasm::WasmSignature *Signature) {
     if (expect(AsmToken::LParen, "("))
       return true;
     if (parseRegTypeList(Signature->Params))
       return true;
     if (expect(AsmToken::RParen, ")"))
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 (CurrentState == Label && WasmSym == LastNonLocalLabel) {
         // This .functype indicates a start of a function.
         if (ensureEmptyNestingStack())
           return true;
         CurrentState = FunctionStart;
-        LastFunctionLabel = LastLabel;
+        LastFunctionLabel = LastNonLocalLabel;
         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: