[WebAssembly] Parsing missing directives to produce valid .o

[aardappel]

Summary

The assembler was able to assemble and then dump back to .s, but
was failing to parse certain directives necessary for valid .o
output:

• .type directives are now recognized to distinguish function symbols and others.
• .size is now parsed to provide function size.
• __stack_pointer is now special-cased with code that is shared with Codegen.

Added test that previously was failing on all 3 of the above.

Also makes a small change in StringSwitch.h for some missing
functionality (being able to detect no match).

[aardappel]

include/llvm/ADT/StringSwitch.h

214 ↗

(On Diff #171596)

Since this is a core LLVM class, should I get someone in particular to review? Or am I better off writing the calling code differently?

include/llvm/MC/MCSymbolWasm.h

73 ↗

(On Diff #171596)

Suggestions welcome where to move this code. The only library/.cpp that Codegen and Assembler share is the WebAssemblyInfo lib, which seemed very unrelated.

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

282 ↗

(On Diff #171596)

I could remove the code below, left in for code review purposes.

[dschuff]

include/llvm/ADT/StringSwitch.h

214 ↗

(On Diff #171596)

For our use case, we have T = wasm::WasmSymbolType and R defaults to T. Could we set R to Optional<wasm::WasmSymbolType> and just use Default instead of NoDefault?

[dschuff]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

282 ↗

(On Diff #171596)

In general, I think making the assembler dumb is not a bad thing. Also you could argue that conventions for symbol names are compiler/ABI conventions and should not be automatically assumed by the assembler. So I guess if we thought of it that way it would mean that we'd have something like this generic code here, and the compiler would be required to output a directive declaring the type of __stack_pointer as a global, right? I think that is actually better than making the assembler smart about the compiler's ABI. Even the factoring of the Assembler/MC code reflects this idea; it's why there's no good place to put that code if it were shared.

[aardappel]

include/llvm/ADT/StringSwitch.h

214 ↗

(On Diff #171596)

That doesn't work because the existing Default always returns a T, i.e. there is no way to indicate a null Optional.

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

282 ↗

(On Diff #171596)

Ok, so I could make the InstPrinter print out this info, but really the only information that should be in the .S output is wether it is global (which is already in there, and I can recover using the commented code below), and the type of the var, which I can also infer in the assembler. So really I don't need to share the function, the only reason to do was that a) it happens the same in both places, and b) that if someone decides to refactor this code they'll be forced to look at both.

I could make the assembler dumber if the InstPrinter also outputs the type. This could be in a directive like .global __stack_pointer i32 but there is no obvious prelude/post output in InstPrinter (on the wasm side). Or we could pack it all in the symbol reference somehow, like __stack_pointer@GLOBAL:I32 (would maybe require changes from the general parser) or __stack_pointer@GLOBAL_I32 (polute with wasm specific symbol types).

[sbc100]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

138 ↗

(On Diff #171596)

LastSymbolType?

145 ↗

(On Diff #171596)

Use = nullptr above instead?

282 ↗

(On Diff #171596)

Seem like either .global __stack_pointer or __stack_pointer@GLOBAL_I32 would be preferable to checkKnownSymbol. For one thing, it would be nice if authors of .S files could declare (or at least use) globals.

Also, .global is probably a bad directive name because .globl already exists for symbol binding.

376 ↗

(On Diff #171596)

Can we limit this to supporting just function and global here? Sections are created using .section directive and I think data is the default.

Then maybe you can avoid the StringSwitch change too.

396 ↗

(On Diff #171596)

Isn't .size already handled by the ELFAsmParser, e.g. ELFAsmParser::ParseDirectiveSize? Or do we not use the ELFAsmParser at all anymore?

[aardappel]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

282 ↗

(On Diff #171596)

Talked with @sunfish also and he seemed to prefer a directive.

What name should we use instead? .wasm_global? That's a bit odd as none of the other directives have wasm_ in them. Maybe .globaltype __stack_pointer i32 makes sense.

396 ↗

(On Diff #171596)

Without me parsing this, the binary writer would complain the size wasn't set. I'll look into it.

[dschuff]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

282 ↗

(On Diff #171596)

I like the directive idea too. ELF has .type (https://sourceware.org/binutils/docs/as/Type.html) for symbol types. I think we have been using it for functions and global variables already as a consequence of our overlap/inheritance of ELF. It seems like this would fit in the same space. Whether we directly use ELF's semantics for the possible types or the ELF implementation code would be another question, but I think conceptually .type makes sense.

[kristina]

include/llvm/ADT/StringSwitch.h

214 ↗

(On Diff #171596)

The general convention is to have an "invalid" type as a default which can be explicitly checked for, something like wasm::WASM_SYMBOL_TYPE_INVALID. IMO if the enum has no default value and yet it's possible for it to end up in an "invalid" state, it *should* have a default value, which makes it much easier to understand the code.

[aardappel]

include/llvm/ADT/StringSwitch.h

214 ↗

(On Diff #171596)

I agree, and most enums have these. I didn't want to add such an enum value here since 0 was already taken by a legal value, and I thought it should be possible to detect that no case applies. But this is the best solution I will add such a value, will need to be value 4 though (enum storage type is unsigned).

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

282 ↗

(On Diff #171596)

We use it as .type test0,@function already, so I could add parsing of .type __stack_pointer,@global,i32 ?

I'd have to output such directives at the end of a .s though, if InstPrinter permits, since I won't know what globals I will encounter at the start.

[aardappel]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

396 ↗

(On Diff #171596)

Yes, ELFAsmParser::ParseDirectiveSize is never called while assembling with llvm-mc.

[aardappel]

Refactored due to code review changes, should be a lot cleaner & simpler now :)

[dschuff]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

196 ↗

(On Diff #172385)

This is pre-existing, so I don't want to hold up this CL, but I think this should only accept v128 and not the MVT types. In the wasm spec there is only a v128 value type (and hence params can only have this one type). In principle LLVM *shouldn't* need to know the MVT in its .param directive, although it's possible that something depends on that now. We should look into this separately. +cc @tlively

351 ↗

(On Diff #172385)

It just occurred to me: do we need to make this stateful like this? Should a .type directive create a symbol/type association regardless of where it is with respect to the actual label? It looks like the ELF Asm parser (ParseDirectiveType) just looks up the symbol (creating if necessary) and emits a symbol attribute to the streamer (which is what actually manipulates the symbol). Maybe we could use this pattern in wasm too, to make asm vs object more uniform?

[dschuff]

I like the idea of trying to make the assembler less stateful if possible, but that can be another CL

[sbc100]

include/llvm/BinaryFormat/Wasm.h

245 ↗

(On Diff #172385)

I'd rather avoid adding this if possible. We don't want to allow symbols to have an invalid type, or have to add this invalid case to our switch statements over symbol type.

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

351 ↗

(On Diff #172385)

This sounds good to me. Than we could avoid LastTypeOfSymbol and GlobalTypeFixups completely?

[aardappel]

Made assembler less stateful thanks to getOrCreateSymbol.

[aardappel]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

196 ↗

(On Diff #172385)

I am fine modifying this either way, but yes, for MVT we don't have a generic type, so it is currently returning v16i8 which is bad.

351 ↗

(On Diff #172385)

Made it less stateful using getOrCreateSymbol.

[sbc100]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

363 ↗

(On Diff #172414)

What isn't ELFAsmParser::ParseDirectiveSize which then calls getStreamer().emitELFSize doing this already?

376 ↗

(On Diff #171596)

So now that you don't need to store Type can you avoid adding WASM_SYMBOL_TYPE_INVALID and just do something like.

if ("function")
  setType(FUNCTION)
else ("global')
  setType(GLOBAL)
else
  error

[sbc100]

otherwise lgtm

[sbc100]

After speaking offline we decided to land this and then followup by investigating how to share more code (or not) with the ELFAsmParser.

[aardappel]

Attempt to fix generic directive parsing, but will do this in a followup.

[aardappel]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

363 ↗

(On Diff #172414)

As discussed offline will do this in a followup, as it requires our own implementation of ELFAsmParser

[aheejin]

This patch addresses global types, but do we have a way to recognize function types for undefined functions? I filed a bug for that: PR39557

[aardappel]

In D53842#1286936, @aheejin wrote:

This patch addresses global types, but do we have a way to recognize function types for undefined functions? I filed a bug for that: PR39557

Thanks! Yes, I am sure there are plenty of directives missing. Effectively we have to encode all metadata the backend generates that is used for constructing .o in them.

llvm/trunk/lib/MC/MCWasmStreamer.cpp

void MCWasmStreamer::EmitAssemblerFlag(MCAssemblerFlag Flag) {
 
   // Do any generic stuff we need to do.
   llvm_unreachable("invalid assembler flag!");
 }
 
 void MCWasmStreamer::ChangeSection(MCSection *Section,
                                    const MCExpr *Subsection) {
   MCAssembler &Asm = getAssembler();
-  auto *SectionWasm = static_cast<const MCSectionWasm *>(Section);
+  auto *SectionWasm = cast<MCSectionWasm>(Section);
   const MCSymbol *Grp = SectionWasm->getGroup();
   if (Grp)
     Asm.registerSymbol(*Grp);
 
   this->MCObjectStreamer::ChangeSection(Section, Subsection);
   Asm.registerSymbol(*Section->getBeginSymbol());
 }
 

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

 #include "llvm/MC/MCContext.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/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;
 
 #define DEBUG_TYPE "wasm-asm-parser"
 
 namespace {
case Symbol:
       break;
     }
   }
 };
 
 class WebAssemblyAsmParser final : public MCTargetAsmParser {
   MCAsmParser &Parser;
   MCAsmLexer &Lexer;
-  MCSymbol *LastLabel;
+  MCSymbolWasm *LastSymbol;
 
 public:
-  WebAssemblyAsmParser(const MCSubtargetInfo &sti, MCAsmParser &Parser,
-                       const MCInstrInfo &mii, const MCTargetOptions &Options)
-      : MCTargetAsmParser(Options, sti, mii), Parser(Parser),
-        Lexer(Parser.getLexer()), LastLabel(nullptr) {
-    setAvailableFeatures(ComputeAvailableFeatures(sti.getFeatureBits()));
+  WebAssemblyAsmParser(const MCSubtargetInfo &STI, MCAsmParser &Parser,
+                       const MCInstrInfo &MII, const MCTargetOptions &Options)
+      : MCTargetAsmParser(Options, STI, MII), Parser(Parser),
+        Lexer(Parser.getLexer()), LastSymbol(nullptr) {
+    setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
   }
 
 #define GET_ASSEMBLER_HEADER
 #include "WebAssemblyGenAsmMatcher.inc"
 
   // TODO: This is required to be implemented, but appears unused.
   bool ParseRegister(unsigned & /*RegNo*/, SMLoc & /*StartLoc*/,
                      SMLoc & /*EndLoc*/) override {
#include "WebAssemblyGenAsmMatcher.inc"
 
   bool Expect(AsmToken::TokenKind Kind, const char *KindName) {
     if (!IsNext(Kind))
       return Error(std::string("Expected ") + KindName + ", instead got: ",
                    Lexer.getTok());
     return false;
   }
 
-  MVT::SimpleValueType ParseRegType(const StringRef &RegType) {
+
+  std::pair<MVT::SimpleValueType, unsigned>
+  ParseRegType(const StringRef &RegType) {
     // Derive type from .param .local decls, or the instruction itself.
-    return StringSwitch<MVT::SimpleValueType>(RegType)
-        .Case("i32", MVT::i32)
-        .Case("i64", MVT::i64)
-        .Case("f32", MVT::f32)
-        .Case("f64", MVT::f64)
-        .Case("i8x16", MVT::v16i8)
-        .Case("i16x8", MVT::v8i16)
-        .Case("i32x4", MVT::v4i32)
-        .Case("i64x2", MVT::v2i64)
-        .Case("f32x4", MVT::v4f32)
-        .Case("f64x2", MVT::v2f64)
+    return StringSwitch<std::pair<MVT::SimpleValueType, unsigned>>(RegType)
+        .Case("i32", {MVT::i32, wasm::WASM_TYPE_I32})
+        .Case("i64", {MVT::i64, wasm::WASM_TYPE_I64})
+        .Case("f32", {MVT::f32, wasm::WASM_TYPE_F32})
+        .Case("f64", {MVT::f64, wasm::WASM_TYPE_F64})
+        .Case("i8x16", {MVT::v16i8, wasm::WASM_TYPE_V128})
+        .Case("i16x8", {MVT::v8i16, wasm::WASM_TYPE_V128})
+        .Case("i32x4", {MVT::v4i32, wasm::WASM_TYPE_V128})
+        .Case("i64x2", {MVT::v2i64, wasm::WASM_TYPE_V128})
+        .Case("f32x4", {MVT::v4f32, wasm::WASM_TYPE_V128})
+        .Case("f64x2", {MVT::v2f64, wasm::WASM_TYPE_V128})
         // arbitrarily chosen vector type to associate with "v128"
         // FIXME: should these be EVTs to avoid this arbitrary hack? Do we want
         // to accept more specific SIMD register types?
-        .Case("v128", MVT::v16i8)
-        .Default(MVT::INVALID_SIMPLE_VALUE_TYPE);
+        .Case("v128", {MVT::v16i8, wasm::WASM_TYPE_V128})
+        .Default({MVT::INVALID_SIMPLE_VALUE_TYPE, wasm::WASM_TYPE_NORESULT});
   }
 
   void ParseSingleInteger(bool IsNegative, OperandVector &Operands) {
     auto &Int = Lexer.getTok();
     int64_t Val = Int.getIntVal();
     if (IsNegative)
       Val = -Val;
     Operands.push_back(make_unique<WebAssemblyOperand>(
bool ParseInstruction(ParseInstructionInfo & /*Info*/, StringRef Name,
     if (BaseName == "block" || BaseName == "loop" || BaseName == "try") {
       Operands.push_back(make_unique<WebAssemblyOperand>(
           WebAssemblyOperand::Integer, NameLoc, NameLoc,
           WebAssemblyOperand::IntOp{-1}));
     }
     return false;
   }
 
-  void onLabelParsed(MCSymbol *Symbol) override { LastLabel = Symbol; }
+  void onLabelParsed(MCSymbol *Symbol) override {
+    LastSymbol = cast<MCSymbolWasm>(Symbol);
+  }
 
   bool ParseDirective(AsmToken DirectiveID) override {
+    // This function has a really weird return value behavior that is different
+    // from all the other parsing functions:
+    // - return true && no tokens consumed -> don't know this directive / let
+    //   the generic parser handle it.
+    // - return true && tokens consumed -> a parsing error occurred.
+    // - return false -> processed this directive successfully.
     assert(DirectiveID.getKind() == AsmToken::Identifier);
     auto &Out = getStreamer();
     auto &TOut =
         reinterpret_cast<WebAssemblyTargetStreamer &>(*Out.getTargetStreamer());
-    // TODO: we're just parsing the subset of directives we're interested in,
-    // and ignoring ones we don't recognise. We should ideally verify
-    // all directives here.
+    // TODO: any time we return an error, at least one token must have been
+    // consumed, otherwise this will not signal an error to the caller.
     if (DirectiveID.getString() == ".type") {
       // This could be the start of a function, check if followed by
       // "label,@function"
-      if (!(IsNext(AsmToken::Identifier) && IsNext(AsmToken::Comma) &&
-            IsNext(AsmToken::At) && Lexer.is(AsmToken::Identifier)))
+      if (!Lexer.is(AsmToken::Identifier))
+        return Error("Expected label after .type directive, got: ",
+                     Lexer.getTok());
+      auto WasmSym = cast<MCSymbolWasm>(
+                       TOut.getStreamer().getContext().getOrCreateSymbol(
+                         Lexer.getTok().getString()));
+      Parser.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")
+        WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
+      else if (TypeName == "global")
+        WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
+      else
+        return Error("Unknown WASM symbol type: ", Lexer.getTok());
+      Parser.Lex();
+      return Expect(AsmToken::EndOfStatement, "EOL");
+    } else if (DirectiveID.getString() == ".size") {
+      if (!Lexer.is(AsmToken::Identifier))
+        return Error("Expected label after .size directive, got: ",
+                     Lexer.getTok());
+      auto WasmSym = cast<MCSymbolWasm>(
+                       TOut.getStreamer().getContext().getOrCreateSymbol(
+                         Lexer.getTok().getString()));
+      Parser.Lex();
+      if (!IsNext(AsmToken::Comma))
+        return Error("Expected `,`, got: ", Lexer.getTok());
+      const MCExpr *Exp;
+      if (Parser.parseExpression(Exp))
+        return Error("Cannot parse .size expression: ", Lexer.getTok());
+      WasmSym->setSize(Exp);
+      return Expect(AsmToken::EndOfStatement, "EOL");
+    } else if (DirectiveID.getString() == ".globaltype") {
+      if (!Lexer.is(AsmToken::Identifier))
+        return Error("Expected symbol name after .globaltype directive, got: ",
+                     Lexer.getTok());
+      auto Name = Lexer.getTok().getString();
+      Parser.Lex();
+      if (!IsNext(AsmToken::Comma))
+        return Error("Expected `,`, got: ", Lexer.getTok());
+      if (!Lexer.is(AsmToken::Identifier))
+        return Error("Expected type in .globaltype directive, got: ",
+                     Lexer.getTok());
+      auto Type = ParseRegType(Lexer.getTok().getString()).second;
+      if (Type == wasm::WASM_TYPE_NORESULT)
+        return Error("Unknown type in .globaltype directive: ",
+                     Lexer.getTok());
       Parser.Lex();
-      // Out.EmitSymbolAttribute(??, MCSA_ELF_TypeFunction);
+      // Now set this symbol with the correct type.
+      auto WasmSym = cast<MCSymbolWasm>(
+                       TOut.getStreamer().getContext().getOrCreateSymbol(Name));
+      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
+      WasmSym->setGlobalType(wasm::WasmGlobalType{uint8_t(Type), true});
+      // And emit the directive again.
+      TOut.emitGlobalType(WasmSym);
+      return Expect(AsmToken::EndOfStatement, "EOL");
     } else if (DirectiveID.getString() == ".param" ||
                DirectiveID.getString() == ".local") {
       // Track the number of locals, needed for correct virtual register
       // assignment elsewhere.
       // Also output a directive to the streamer.
       std::vector<MVT> Params;
       std::vector<MVT> Locals;
       while (Lexer.is(AsmToken::Identifier)) {
-        auto RegType = ParseRegType(Lexer.getTok().getString());
+        auto RegType = ParseRegType(Lexer.getTok().getString()).first;
         if (RegType == MVT::INVALID_SIMPLE_VALUE_TYPE)
           return true;
         if (DirectiveID.getString() == ".param") {
           Params.push_back(RegType);
         } else {
           Locals.push_back(RegType);
         }
         Parser.Lex();
         if (!IsNext(AsmToken::Comma))
           break;
       }
-      assert(LastLabel);
-      TOut.emitParam(LastLabel, Params);
+      assert(LastSymbol);
+      // TODO: LastSymbol isn't even used by emitParam, so could be removed.
+      TOut.emitParam(LastSymbol, Params);
       TOut.emitLocal(Locals);
+      return Expect(AsmToken::EndOfStatement, "EOL");
     } else {
-      // For now, ignore anydirective we don't recognize:
+      // TODO: remove.
       while (Lexer.isNot(AsmToken::EndOfStatement))
         Parser.Lex();
-    }
       return Expect(AsmToken::EndOfStatement, "EOL");
     }
+    // TODO: current ELF directive parsing is broken, fix this is a followup.
+    //return true;  // We didn't process this directive.
+    return false;
+  }
 
   bool MatchAndEmitInstruction(SMLoc IDLoc, unsigned & /*Opcode*/,
                                OperandVector &Operands, MCStreamer &Out,
                                uint64_t &ErrorInfo,
                                bool MatchingInlineAsm) override {
     MCInst Inst;
     unsigned MatchResult =
         MatchInstructionImpl(Operands, Inst, ErrorInfo, MatchingInlineAsm);

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

 # RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+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=+simd128,+nontrapping-fptoint,+exception-handling < %s
 
     .text
     .type    test0,@function
 test0:
     # Test all types:
     .param      i32, i64
     .local      f32, f64, v128, v128
     # Explicit getlocal/setlocal:
.LBB0_3:
     i32.catch   0
 .LBB0_4:
     catch_all
 .LBB0_5:
     end_try
     #i32.trunc_s:sat/f32
     get_global  __stack_pointer@GLOBAL
     end_function
-
+.Lfunc_end0:
+  .size test0, .Lfunc_end0-test0
+    .globaltype __stack_pointer, i32
 
 # CHECK:           .text
 # CHECK-LABEL: test0:
 # CHECK-NEXT:      .param      i32, i64
 # CHECK-NEXT:      .local      f32, f64
 # CHECK-NEXT:      get_local   2
 # CHECK-NEXT:      set_local   2
 # CHECK-NEXT:      i32.const   -1

 # CHECK-NEXT:  .LBB0_3:
 # CHECK-NEXT:      i32.catch   0
 # CHECK-NEXT:  .LBB0_4:
 # CHECK-NEXT:      catch_all
 # CHECK-NEXT:  .LBB0_5:
 # CHECK-NEXT:      end_try
 # CHECK-NEXT:      get_global  __stack_pointer@GLOBAL
 # CHECK-NEXT:      end_function
+
+# CHECK:           .globaltype  __stack_pointer, i32