[WebAssembly] Make segment/size/type directives optional in asm

[aardappel]

Summary

These were "boilerplate" that repeated information already present
in .functype and end_function, that needed to be repeated to Please
the particular way our object writing works, and missing them would
generate errors.

Instead, we generate the information for these automatically so the
user can concern itself with writing more canonical wasm functions
that always work as expected.

[sbc100]

lib/MC/MCParser/WasmAsmParser.cpp

97 ↗

(On Diff #184619)

Why not just remove this? Same below.

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

680 ↗

(On Diff #184619)

Remove curlies?

[aardappel]

lib/MC/MCParser/WasmAsmParser.cpp

97 ↗

(On Diff #184619)

I thought I'd leave this in at least for review, and potentially also to show "this is what this operation would do", since it is kinda odd that we parse the directive, then don't do anything with it.

I mean, I could remove all the parsing code and replace it with code that skips this directive, but I thought I'd leave it in, should we have a need for it later.

[aheejin]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

711 ↗

(On Diff #184619)

clang-format :) other parts too

[aardappel]

code review fixes, and simplified end_function check.

[dschuff]

lib/MC/MCParser/WasmAsmParser.cpp

97 ↗

(On Diff #184619)

Is it really true that we never use the section directive? I guess for functions that makes sense but what about for data sections?

[dschuff]

lib/MC/MCParser/WasmAsmParser.cpp

97 ↗

(On Diff #184619)

WRT leaving in the commented-out code, the text comment could at least say something like "here is what *would* be here otherwise" so it's clear that the code is there intentionally.

115 ↗

(On Diff #184643)

This comment can go now, right?

[dschuff]

otherwise LGTM too

[aardappel]

lib/MC/MCParser/WasmAsmParser.cpp

97 ↗

(On Diff #184619)

We currently don't, as data sections aren't implemented yet. I'll revise this code if/when I implement those. And yes, I'll change the comment.

[aardappel]

code review fixes

llvm/trunk/lib/MC/MCParser/WasmAsmParser.cpp

bool parseSectionDirective(StringRef, SMLoc) {
     StringRef Name;
     if (Parser->parseIdentifier(Name))
       return TokError("expected identifier in directive");
     // FIXME: currently requiring this very fixed format.
     if (Expect(AsmToken::Comma, ",") || Expect(AsmToken::String, "string") ||
         Expect(AsmToken::Comma, ",") || Expect(AsmToken::At, "@") ||
         Expect(AsmToken::EndOfStatement, "eol"))
       return true;
-    auto WS = getContext().getWasmSection(Name, SectionKind::getText());
-    getStreamer().SwitchSection(WS);
+    // This is done automatically by the assembler for text sections currently,
+    // so we don't need to emit that here. This is what it would do (and may
+    // be needed later for other section types):
+    // auto WS = getContext().getWasmSection(Name, SectionKind::getText());
+    // 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;
     if (Parser->parseIdentifier(Name))
       return TokError("expected identifier in directive");
     auto Sym = getContext().getOrCreateSymbol(Name);
     if (Expect(AsmToken::Comma, ","))
       return true;
     const MCExpr *Expr;
     if (Parser->parseExpression(Expr))
       return true;
     if (Expect(AsmToken::EndOfStatement, "eol"))
       return true;
-    // MCWasmStreamer implements this.
-    getStreamer().emitELFSize(Sym, Expr);
+    // This is done automatically by the assembler for functions currently,
+    // so we don't need to emit that here. This is what it would do:
+    (void)Sym;
+    // getStreamer().emitELFSize(Sym, Expr);
     return false;
   }
 
   bool parseDirectiveType(StringRef, SMLoc) {
     // This could be the start of a function, check if followed by
     // "label,@function"
     if (!Lexer->is(AsmToken::Identifier))
       return Error("Expected label after .type directive, got: ",

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

 /// It contains code to translate a parsed .s file into MCInsts.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/WebAssemblyMCTargetDesc.h"
 #include "MCTargetDesc/WebAssemblyTargetStreamer.h"
 #include "WebAssembly.h"
 #include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"
 #include "llvm/MC/MCParser/MCTargetAsmParser.h"
+#include "llvm/MC/MCSectionWasm.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCSymbolWasm.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/TargetRegistry.h"
 
 using namespace llvm;
class WebAssemblyAsmParser final : public MCTargetAsmParser {
   // (the streamer does not enforce this). This "state machine" enum helps
   // guarantee that correct order.
   enum ParserState {
     FileStart,
     Label,
     FunctionStart,
     FunctionLocals,
     Instructions,
+    EndFunction,
   } 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()));
   }
if (BaseName == "block") {
         return true;
     } else if (BaseName == "end_loop") {
       if (pop(BaseName, Loop))
         return true;
     } else if (BaseName == "end_block") {
       if (pop(BaseName, Block))
         return true;
     } else if (BaseName == "end_function") {
+      CurrentState = EndFunction;
       if (pop(BaseName, Function) || ensureEmptyNestingStack())
         return true;
     }
 
     while (Lexer.isNot(AsmToken::EndOfStatement)) {
       auto &Tok = Lexer.getTok();
       switch (Tok.getKind()) {
       case AsmToken::Identifier: {
if (DirectiveID.getString() == ".functype") {
       if (SymName.empty())
         return true;
       auto WasmSym = cast<MCSymbolWasm>(Ctx.getOrCreateSymbol(SymName));
       if (CurrentState == Label && WasmSym == LastLabel) {
         // This .functype indicates a start of a function.
         if (ensureEmptyNestingStack())
           return true;
         CurrentState = FunctionStart;
+        LastFunctionLabel = LastLabel;
         push(Function);
       }
       auto Signature = 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);
case Match_Success: {
         // This is the first instruction in a function, but we haven't seen
         // a .local directive yet. The streamer requires locals to be encoded
         // as a prelude to the instructions, so emit an empty list of locals
         // here.
         auto &TOut = reinterpret_cast<WebAssemblyTargetStreamer &>(
             *Out.getTargetStreamer());
         TOut.emitLocal(SmallVector<wasm::ValType, 0>());
       }
-      CurrentState = Instructions;
       Out.EmitInstruction(Inst, getSTI());
+      if (CurrentState == EndFunction) {
+        onEndOfFunction();
+      } else {
+        CurrentState = Instructions;
+      }
       return false;
     }
     case Match_MissingFeature:
       return Parser.Error(
           IDLoc, "instruction requires a WASM feature not currently enabled");
     case Match_MnemonicFail:
       return Parser.Error(IDLoc, "invalid instruction");
     case Match_NearMisses:
case Match_InvalidOperand: {
           ErrorLoc = IDLoc;
       }
       return Parser.Error(ErrorLoc, "invalid operand for instruction");
     }
     }
     llvm_unreachable("Implement any new match types added!");
   }
 
+  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.
+    auto SecName = ".text." + SymName;
+    auto WS = getContext().getWasmSection(SecName, SectionKind::getText());
+    getStreamer().SwitchSection(WS);
+  }
+
+  void onEndOfFunction() {
+    // Automatically output a .size directive, so it becomes optional for the
+    // user.
+    auto TempSym = getContext().createLinkerPrivateTempSymbol();
+    getStreamer().EmitLabel(TempSym);
+    auto Start = MCSymbolRefExpr::create(LastLabel, getContext());
+    auto End = MCSymbolRefExpr::create(TempSym, getContext());
+    auto Expr =
+        MCBinaryExpr::create(MCBinaryExpr::Sub, End, Start, getContext());
+    getStreamer().emitELFSize(LastFunctionLabel, Expr);
+  }
+
   void onEndOfFile() override { ensureEmptyNestingStack(); }
 };
 } // end anonymous namespace
 
 // Force static initialization.
 extern "C" void LLVMInitializeWebAssemblyAsmParser() {
   RegisterMCAsmParser<WebAssemblyAsmParser> X(getTheWebAssemblyTarget32());
   RegisterMCAsmParser<WebAssemblyAsmParser> Y(getTheWebAssemblyTarget64());
 }
 
 #define GET_REGISTER_MATCHER
 #define GET_MATCHER_IMPLEMENTATION
 #include "WebAssemblyGenAsmMatcher.inc"

llvm/trunk/test/MC/WebAssembly/assembler-binary.ll

 
 ; ASM:      .text
 ; ASM:        .file "assembler-binary.ll"
 ; ASM:        .globl  foo
 ; ASM:      foo:
 ; ASM-NEXT:   .functype foo (i32) -> ()
 ; ASM-NEXT:   call  bar@FUNCTION
 ; ASM-NEXT:   end_function
-; ASM-NEXT: .Lfunc_end0:
-; ASM-NEXT:   .size foo, .Lfunc_end0-foo
 ; ASM:        .functype bar () -> ()
 
 
 ; CHECK:      --- !WASM
 ; CHECK-NEXT: FileHeader:
 ; CHECK-NEXT:   Version:         0x00000001
 ; CHECK-NEXT: Sections:
 ; CHECK-NEXT:   - Type:            TYPE

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

 # RUN: not llvm-mc -triple=wasm32-unknown-unknown -mattr=+simd128,+nontrapping-fptoint,+exception-handling < %s 2>&1 | FileCheck %s
 
-    .text
-    .section    .text.main,"",@
-    .type       test0,@function
 # CHECK: End of block construct with no start: end_try
     end_try
 test0:
     .functype   test0 () -> ()
 # CHECK: Block construct type mismatch, expected: end_function, instead got: end_loop
     end_loop
     block
 # CHECK: Block construct type mismatch, expected: end_block, instead got: end_if
     end_if
     try
     loop
 # CHECK: Block construct type mismatch, expected: end_loop, instead got: end_function
 # CHECK: error: Unmatched block construct(s) at function end: loop
 # CHECK: error: Unmatched block construct(s) at function end: try
 # CHECK: error: Unmatched block construct(s) at function end: block
 # CHECK: error: Unmatched block construct(s) at function end: function
     end_function
-.Lfunc_end0:
-    .size       test0, .Lfunc_end0-test0
 

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

 # RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+unimplemented-simd128,+nontrapping-fptoint,+exception-handling < %s | FileCheck %s
 # this one is just here to see if it converts to .o without errors, but doesn't check any output:
 # RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj -mattr=+unimplemented-simd128,+nontrapping-fptoint,+exception-handling < %s
 
-    .text
-    .section .text.main,"",@
-    .type    test0,@function
 test0:
     # 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
.LBB0_4:
     end_block
     end_try
     i32.const 0
     throw 0
 .LBB0_5:
     #i32.trunc_sat_f32_s
     global.get  __stack_pointer@GLOBAL
     end_function
-.Lfunc_end0:
-    .size test0, .Lfunc_end0-test0
     .globaltype __stack_pointer, i32
 
 # CHECK:           .text
 # CHECK-LABEL: test0:
 # CHECK-NEXT:      .functype test0 (i32, i64) -> (i32)
 # CHECK-NEXT:      .eventtype  __cpp_exception i32
 # CHECK-NEXT:      .local      f32, f64
 # CHECK-NEXT:      local.get   2

llvm/trunk/test/MC/WebAssembly/objdump.s

 # RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj -o %t.o -mattr=+simd128,+nontrapping-fptoint,+exception-handling < %s
 # RUN: llvm-objdump -triple=wasm32-unknown-unknown -disassemble %t.o | FileCheck %s
 
-    .section .text.main1,"",@
-    .type    test0,@function
 test0:
     .functype   test0 (i32, i64) -> (i32)
     .local      f32, f64, v128, v128
     local.get   2
     end_function
-.Lfunc_end0:
-    .size test0, .Lfunc_end0-test0
 
-    .section .text.main2,"",@
-    .type    test1,@function
 test1:
     .functype   test1 (i32, i64) -> (i32)
     .local      i32, i64, except_ref
     local.get   3
     end_function
-.Lfunc_end1:
-    .size test1, .Lfunc_end1-test1
 
 
 # CHECK-LABEL: CODE:
 # CHECK:             # 2 functions in section.
 # CHECK-LABEL: test0:
 # CHECK-NEXT:        .local  f32, f64, v128, v128
 # CHECK-NEXT:       9:       20 02  local.get 2
 # CHECK-NEXT:       b:       0b     end_block
 # CHECK-LABEL: test1:
 # CHECK-NEXT:        .local  i32, i64, except_ref
 # CHECK-NEXT:      14:       20 03  local.get 3
 # CHECK-NEXT:      16:       0b     end_block