[WebAssembly] Enforce assembler emits to streamer in order.

[aardappel]

Summary

The assembler processes directives and instructions in whatever order
they are in the file, then directly emits them to the streamer. This
could cause badly written (or generated) .s files to produce
incorrect binaries.

It now has state that tracks what it has most recently seen, to
enforce they are emitted in a given order that always produces
correct wasm binaries.

Also added a new test that compares obj2yaml output from llc (the
backend) to that going via .s and the assembler to ensure both paths
generate the same binaries.

The features this test covers could be extended.

Passes all wasm Lit tests.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=39557

[aardappel]

This took me a while to figure out what was going wrong (lack of .local in a file caused emitLocal not to be called which produced invalid wasm code sections...)

[aardappel]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

408 ↗

(On Diff #176212)

This code is not super elegant, but kinda has to be this way because we have .functype which both introduces a function and ones which just declare external ones. Now if you misspel a label you'll fail to start a new function.

More ideal would be split these, and have a .functype with no name that annotates the start of a function (and the assembler enforces only comes after a label) and a different one, e.g. .funcdecl, that does have its own name and is only used for external types. Not sure if such a refactoring is worth it though, after we just re-did all the tests with .functype.

[aheejin]

Style nits

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

144 ↗

(On Diff #176212)

Nit: Could you wrap comments to 80 cols? (Currently some lines end prematurely)

150 ↗

(On Diff #176212)

clang-format this?

161 ↗

(On Diff #176212)

Nit: Can we use C++11-style in-class initialization for these two variables?

400 ↗

(On Diff #176212)

Nit: Comment 80-col wrapping too, and maybe other parts as well

450 ↗

(On Diff #176212)

Is this clang-formatted?

[aheejin]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

149 ↗

(On Diff #176212)

This does not seem to be used

test/MC/WebAssembly/assembler-binary.ll

1 ↗

(On Diff #176212)

Do we need to check all these asm and yaml outputs in this test? Wouldn't yaml output of function symbol related parts be sufficient?

92 ↗

(On Diff #176212)

Trailing newline

[aardappel]

Code review fixes.

[aardappel]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

150 ↗

(On Diff #176212)

I did, it doesn't change :)

161 ↗

(On Diff #176212)

Sure.. I guess I was being conservative, not knowing what level of C++ I can use.

400 ↗

(On Diff #176212)

There's not much to wrap here, I wanted TODO to start on its own line.

408 ↗

(On Diff #176212)

Also note that in the past we decided that we like the assembler to be relatively "dumb", in terms of not needing to have a lot of structural knowledge about wasm. Turns out that is not possible with how streamers work, we need the assembler to enforce a certain level of structure.

test/MC/WebAssembly/assembler-binary.ll

1 ↗

(On Diff #176212)

Yes, but then when the yaml test breaks on e.g. the code bytes, it will be hard to track down which instruction is causing it. With the asm tests, those will break first so it will be easier to fix.

[sbc100]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

155 ↗

(On Diff #176212)

= nullptr here?

396 ↗

(On Diff #176212)

This sentence doesn't make sense. typo?

408 ↗

(On Diff #176212)

I like the repeated name in the .functype directive because it seems to match existing assembly directives.

If we want to distinguish between types for internal vs external functions perhaps we would want a new directive such as ".extern_functype" but as you say I'm not sure its worth it.

[dschuff]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

161 ↗

(On Diff #176212)

In LLVM, we love ALL the C++! but there's still those pesky older versions of compilers, distros, etc. see https://llvm.org/docs/CodingStandards.html#languages-libraries-and-standards . See also http://lists.llvm.org/pipermail/llvm-dev/2018-October/127045.html

[aardappel]

test/MC/WebAssembly/assembler-binary.ll

1 ↗

(On Diff #176212)

It would be awesome for obj2yaml to be able to disassemble its code section, so we could have more comprehensive binary tests that are readable.

[aardappel]

lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

161 ↗

(On Diff #176212)

Yes, I use C++17 on some projects, C++0x (pre-C++11) on others, so I tend to get confused on what I can use. I'll re-read that section to remind myself :)

408 ↗

(On Diff #176212)

Ok, will leave it as-is unless anyone else feels strongly about it.

[aardappel]

Code review fixes.

[sbc100]

test/MC/WebAssembly/assembler-binary.ll

1 ↗

(On Diff #176446)

Hmm, rather than running llc twice here you could do

; RUN: llc -filetype=asm -asm-verbose=false %s -o %t.s
; RUN: FileCheck -check-prefix=ASM --input-file %t.s
; RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=asm %t.s -o - | FileCheck -check-prefix=ASM %s

Basically write the .s to a temp file and use that rather then generating and piping it twice.

I'm really not sure which is better but I thought I'd mention it as an option :)

[aardappel]

Optimized test runs of llc.

[aardappel]

test/MC/WebAssembly/assembler-binary.ll

1 ↗

(On Diff #176446)

Good idea! Removes 3 runs of llc. Though rather than --input-file %t.s it needs -input-file %t.s %s.

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

 
 class WebAssemblyAsmParser final : public MCTargetAsmParser {
   MCAsmParser &Parser;
   MCAsmLexer &Lexer;
 
   // Much like WebAssemblyAsmPrinter in the backend, we have to own these.
   std::vector<std::unique_ptr<wasm::WasmSignature>> Signatures;
 
+  // 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,
+  } CurrentState = FileStart;
+
+  // 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;
+
 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 (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;
+    CurrentState = Label;
+  }
+
   // This function processes wasm-specific directives streamed to
   // WebAssemblyTargetStreamer, all others go to the generic parser
   // (see WasmAsmParser).
   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.
if (DirectiveID.getString() == ".globaltype") {
                     TOut.getStreamer().getContext().getOrCreateSymbol(SymName));
       WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
       WasmSym->setGlobalType(
             wasm::WasmGlobalType{uint8_t(Type.getValue()), true});
       // And emit the directive again.
       TOut.emitGlobalType(WasmSym);
       return Expect(AsmToken::EndOfStatement, "EOL");
     } else if (DirectiveID.getString() == ".functype") {
+      // This code has to send things to the streamer similar to
+      // 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 seperately.
       auto SymName = ExpectIdent();
       if (SymName.empty()) return true;
       auto WasmSym = cast<MCSymbolWasm>(
                     TOut.getStreamer().getContext().getOrCreateSymbol(SymName));
+      if (CurrentState == Label && WasmSym == LastLabel) {
+        // This .functype indicates a start of a function.
+        CurrentState = FunctionStart;
+      }
       auto Signature = make_unique<wasm::WasmSignature>();
       if (Expect(AsmToken::LParen, "(")) return true;
       if (ParseRegTypeList(Signature->Params)) return true;
       if (Expect(AsmToken::RParen, ")")) return true;
       if (Expect(AsmToken::MinusGreater, "->")) return true;
       if (Expect(AsmToken::LParen, "(")) return true;
       if (ParseRegTypeList(Signature->Returns)) return true;
       if (Expect(AsmToken::RParen, ")")) return true;
       WasmSym->setSignature(Signature.get());
       addSignature(std::move(Signature));
       WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
       TOut.emitFunctionType(WasmSym);
+      // TODO: backend also calls TOut.emitIndIdx, but that is not implemented.
       return Expect(AsmToken::EndOfStatement, "EOL");
     } else if (DirectiveID.getString() == ".local") {
+      if (CurrentState != FunctionStart)
+        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");
     }
     return true;  // We didn't process this directive.
   }
 
   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);
     switch (MatchResult) {
     case Match_Success: {
+      if (CurrentState == FunctionStart) {
+        // 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());
       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");

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

+; RUN: llc -filetype=asm -asm-verbose=false %s -o %t.s
+; RUN: FileCheck -check-prefix=ASM -input-file %t.s %s
+; RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=asm %t.s -o - | FileCheck -check-prefix=ASM %s
+; RUN: llc -filetype=obj %s -o - | obj2yaml | FileCheck %s
+; RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj %t.s -o - | obj2yaml | FileCheck %s
+
+; This specifically tests that we can generate a binary from the assembler
+; that produces the same binary as the backend would.
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+declare void @bar()
+
+define void @foo(i32 %n) {
+entry:
+  call void @bar()
+  ret void
+}
+
+; Checking assembly is not the point of this test, but if something breaks
+; it is easier to spot it here than in the yaml output.
+
+; 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
+; CHECK-NEXT:     Signatures:
+; CHECK-NEXT:       - Index:           0
+; CHECK-NEXT:         ReturnType:      NORESULT
+; CHECK-NEXT:         ParamTypes:
+; CHECK-NEXT:           - I32
+; CHECK-NEXT:       - Index:           1
+; CHECK-NEXT:         ReturnType:      NORESULT
+; CHECK-NEXT:         ParamTypes:      []
+; CHECK-NEXT:   - Type:            IMPORT
+; CHECK-NEXT:     Imports:
+; CHECK-NEXT:       - Module:          env
+; CHECK-NEXT:         Field:           __linear_memory
+; CHECK-NEXT:         Kind:            MEMORY
+; CHECK-NEXT:         Memory:
+; CHECK-NEXT:           Initial:         0x00000000
+; CHECK-NEXT:       - Module:          env
+; CHECK-NEXT:         Field:           __indirect_function_table
+; CHECK-NEXT:         Kind:            TABLE
+; CHECK-NEXT:         Table:
+; CHECK-NEXT:           ElemType:        ANYFUNC
+; CHECK-NEXT:           Limits:
+; CHECK-NEXT:             Initial:         0x00000000
+; CHECK-NEXT:       - Module:          env
+; CHECK-NEXT:         Field:           bar
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         SigIndex:        1
+; CHECK-NEXT:   - Type:            FUNCTION
+; CHECK-NEXT:     FunctionTypes:   [ 0 ]
+; CHECK-NEXT:   - Type:            CODE
+; CHECK-NEXT:     Relocations:
+; CHECK-NEXT:       - Type:            R_WEBASSEMBLY_FUNCTION_INDEX_LEB
+; CHECK-NEXT:         Index:           1
+; CHECK-NEXT:         Offset:          0x00000004
+; CHECK-NEXT:     Functions:
+; CHECK-NEXT:       - Index:           1
+; CHECK-NEXT:         Locals:          []
+; CHECK-NEXT:         Body:            1080808080000B
+; CHECK-NEXT:   - Type:            CUSTOM
+; CHECK-NEXT:     Name:            linking
+; CHECK-NEXT:     Version:         1
+; CHECK-NEXT:     SymbolTable:
+; CHECK-NEXT:       - Index:           0
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Name:            foo
+; CHECK-NEXT:         Flags:           [  ]
+; CHECK-NEXT:         Function:        1
+; CHECK-NEXT:       - Index:           1
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Name:            bar
+; CHECK-NEXT:         Flags:           [ UNDEFINED ]
+; CHECK-NEXT:         Function:        0
+; CHECK-NEXT: ...