[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

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

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

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

[dschuff]

include/llvm/ADT/StringSwitch.h

214

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

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

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

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

LastSymbolType?

145

Use = nullptr above instead?

282

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

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

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

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

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

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

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

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

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

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

195

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

364–370

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

364–370

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

195

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.

364–370

Made it less stateful using getOrCreateSymbol.

[sbc100]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

376

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

393

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

[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

393

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.

include/llvm/ADT/StringSwitch.h

public:
   LLVM_NODISCARD
   LLVM_ATTRIBUTE_ALWAYS_INLINE
   R Default(T Value) {
     if (Result)
       return std::move(*Result);
     return Value;
   }
 
+  // Useful if the target enum has no "undefined" value.
+  LLVM_NODISCARD
+  LLVM_ATTRIBUTE_ALWAYS_INLINE
+  Optional<T> NoDefault() {
+    return std::move(Result);
+  }
+
   LLVM_NODISCARD
   LLVM_ATTRIBUTE_ALWAYS_INLINE
   operator R() {
     assert(Result && "Fell off the end of a string-switch");
     return std::move(*Result);
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_ADT_STRINGSWITCH_H

include/llvm/MC/MCSymbolWasm.h

const wasm::WasmGlobalType &getGlobalType() const {
     assert(GlobalTypeSet);
     return GlobalType;
   }
 
   void setGlobalType(wasm::WasmGlobalType GT) {
     GlobalTypeSet = true;
     GlobalType = GT;
   }
+
+  // TODO(wvo): not great to have this code in a header, but currently not a
+  // a lot of other places shared by Codegen and Assembler.
+  bool checkKnownSymbol(StringRef Name, bool HasAddr64) {
+    // __stack_pointer is a global variable; all other external symbols used by
+    // CodeGen are functions.  It's OK to hardcode knowledge of specific symbols
+    // here; this method is precisely there for fetching the signatures of known
+    // Clang-provided symbols.
+    if (Name == "__stack_pointer") {
+      setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
+      setGlobalType(wasm::WasmGlobalType{
+          uint8_t(HasAddr64 ? wasm::WASM_TYPE_I64 : wasm::WASM_TYPE_I32),
+          true});
+      return true;
+    }
+    return false;
+  }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCSYMBOLWASM_H

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;
+  MCSymbolWasm *LastFunction;
+  std::string LastTypedSymbolName;
+  wasm::WasmSymbolType LastTypeOfSymbol;
+  const MCSubtargetInfo &STI;
 
 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), LastFunction(nullptr),
+        LastTypeOfSymbol(wasm::WASM_SYMBOL_TYPE_FUNCTION), STI(STI) {
+    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 {
return StringSwitch<MVT::SimpleValueType>(RegType)
         .Case("i64x2", MVT::v2i64)
         .Case("f32x4", MVT::v4f32)
         .Case("f64x2", MVT::v2f64)
         // 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);
   }
 
   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>(
         WebAssemblyOperand::Integer, Int.getLoc(), Int.getEndLoc(),
while (Lexer.isNot(AsmToken::EndOfStatement)) {
       auto &Tok = Lexer.getTok();
       switch (Tok.getKind()) {
       case AsmToken::Identifier: {
         auto &Id = Lexer.getTok();
         const MCExpr *Val;
         SMLoc End;
         if (Parser.parsePrimaryExpr(Val, End))
           return Error("Cannot parse symbol: ", Lexer.getTok());
+        if (Val->getKind() == MCExpr::SymbolRef) {
+          auto SymRef = cast<MCSymbolRefExpr>(Val);
+          // TODO: there has to be a more elegant way?
+          auto &WasmSymbol = cast<MCSymbolWasm>(
+                              const_cast<MCSymbol &>(SymRef->getSymbol()));
+          // See if this is "__stack_pointer" or some other symbol we treat
+          // specially.
+          WasmSymbol.checkKnownSymbol(WasmSymbol.getName(), STI.getTargetTriple().getArch() ==
+                                            Triple::wasm64);
+          // TODO: do we ideally want more generic handling like this code
+          // below instead of checkKnownSymbol?
+          /*
+          switch (SymRef->getKind()) {
+            case MCSymbolRefExpr::VK_WebAssembly_FUNCTION:
+              WasmSymbol.setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
+              break;
+            case MCSymbolRefExpr::VK_WebAssembly_GLOBAL:
+              WasmSymbol.setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
+              break;
+            default:;
+          }
+          */
+        }
         Operands.push_back(make_unique<WebAssemblyOperand>(
             WebAssemblyOperand::Symbol, Id.getLoc(), Id.getEndLoc(),
             WebAssemblyOperand::SymOp{Val}));
         break;
       }
       case AsmToken::Minus:
         Parser.Lex();
         if (Lexer.isNot(AsmToken::Integer))
bool ParseInstruction(ParseInstructionInfo & /*Info*/, StringRef Name,
     if (BaseName == "block" || BaseName == "loop") {
       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 {
+    auto WasmSymbol = cast<MCSymbolWasm>(Symbol);
+    LastSymbol = WasmSymbol;
+    if (LastTypedSymbolName == WasmSymbol->getName()) {
+      WasmSymbol->setType(LastTypeOfSymbol);
+      if (LastTypeOfSymbol == wasm::WASM_SYMBOL_TYPE_FUNCTION)
+        LastFunction = WasmSymbol;
+    }
+  }
 
   bool ParseDirective(AsmToken DirectiveID) override {
     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.
     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());
+      LastTypedSymbolName = 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 Type = StringSwitch<wasm::WasmSymbolType>(Lexer.getTok().getString())
+                  .Case("function", wasm::WASM_SYMBOL_TYPE_FUNCTION)
+                  .Case("data", wasm::WASM_SYMBOL_TYPE_DATA)
+                  .Case("global", wasm::WASM_SYMBOL_TYPE_GLOBAL)
+                  .Case("section", wasm::WASM_SYMBOL_TYPE_SECTION)
+                  .NoDefault();
+      if (!Type)
+        return Error("Unknown WASM symbol type: ", Lexer.getTok());
+      Parser.Lex();
+      LastTypeOfSymbol = *Type;
+    } else if (DirectiveID.getString() == ".size") {
+      if (!Lexer.is(AsmToken::Identifier))
+        return Error("Expected label after .size directive, got: ",
+                     Lexer.getTok());
+      if (!LastFunction ||
+          LastFunction->getName() != Lexer.getTok().getString())
+        return Error(".size directive without corresponding function start with"
+                     " .type for: ", Lexer.getTok());
       Parser.Lex();
-      // Out.EmitSymbolAttribute(??, MCSA_ELF_TypeFunction);
+      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());
+      LastFunction->setSize(Exp);
     } 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());
         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);
+      TOut.emitParam(LastSymbol, Params);
       TOut.emitLocal(Locals);
     } else {
       // For now, ignore anydirective we don't recognize:
       while (Lexer.isNot(AsmToken::EndOfStatement))
         Parser.Lex();
     }
     return Expect(AsmToken::EndOfStatement, "EOL");
   }

lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

 
 MCSymbol *WebAssemblyMCInstLower::GetExternalSymbolSymbol(
     const MachineOperand &MO) const {
   const char *Name = MO.getSymbolName();
   MCSymbolWasm *WasmSym =
       cast<MCSymbolWasm>(Printer.GetExternalSymbolSymbol(Name));
   const WebAssemblySubtarget &Subtarget = Printer.getSubtarget();
 
-  // __stack_pointer is a global variable; all other external symbols used by
-  // CodeGen are functions.  It's OK to hardcode knowledge of specific symbols
-  // here; this method is precisely there for fetching the signatures of known
-  // Clang-provided symbols.
-  if (strcmp(Name, "__stack_pointer") == 0) {
-    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
-    WasmSym->setGlobalType(wasm::WasmGlobalType{
-        uint8_t(Subtarget.hasAddr64() ? wasm::WASM_TYPE_I64
-                                      : wasm::WASM_TYPE_I32),
-        true});
-    return WasmSym;
-  }
+  // See if this is "__stack_pointer" or some other symbol we treat specially.
+  if (WasmSym->checkKnownSymbol(Name, Subtarget.hasAddr64())) return WasmSym;
 
   SmallVector<wasm::ValType, 4> Returns;
   SmallVector<wasm::ValType, 4> Params;
   GetLibcallSignature(Subtarget, Name, Returns, Params);
   auto Signature =
       make_unique<wasm::WasmSignature>(std::move(Returns), std::move(Params));
   WasmSym->setSignature(Signature.get());
   Printer.addSignature(std::move(Signature));

test/MC/WebAssembly/basic-assembly.s

 # RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+simd128,+nontrapping-fptoint < %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 < %s
 
     .text
     .type    test0,@function
 test0:
     # Test all types:
     .param      i32, i64
     .local      f32, f64, v128, v128
     # Explicit getlocal/setlocal:
.LBB0_2:
     f32x4.add
     # Test correct parsing of instructions with / and : in them:
     # TODO: enable once instruction has been added.
     #i32x4.trunc_s/f32x4:sat
     i32.trunc_s/f32
     #i32.trunc_s:sat/f32
     get_global  __stack_pointer@GLOBAL
     end_function
-
+.Lfunc_end0:
+  .size test0, .Lfunc_end0-test0
 
 # 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