[WebAssembly][MC] Improve error handling on empty function declarations

[sbc100]

Summary

Fixes: #57427

[dschuff]

IIUC in other asm formats, you can just e.g. switch to the data section (in the middle of generating code for a function), emit a data constant, and then switch back and finish the function. In theory we could do that for wasm too, as long as we do actually switch back and finish the function. but I get that would make this error checking a lot more complicated, and I don't know of any cases where we would actually want to do that. do you?

[sbc100]

In D133057#3762475, @dschuff wrote:

IIUC in other asm formats, you can just e.g. switch to the data section (in the middle of generating code for a function), emit a data constant, and then switch back and finish the function. In theory we could do that for wasm too, as long as we do actually switch back and finish the function. but I get that would make this error checking a lot more complicated, and I don't know of any cases where we would actually want to do that. do you?

I can't think of any no.. but at the same time I didn't indent this change to change that behaviour. I will check if that is possible prior to this change.. if it is possible, we can reconsider.

[sbc100]

In D133057#3762475, @dschuff wrote:

IIUC in other asm formats, you can just e.g. switch to the data section (in the middle of generating code for a function), emit a data constant, and then switch back and finish the function. In theory we could do that for wasm too, as long as we do actually switch back and finish the function. but I get that would make this error checking a lot more complicated, and I don't know of any cases where we would actually want to do that. do you?

I looked into this and I think we already can't support this type of thing in the general case due to the way to type checking works.

For example there is global state in the type checker that assumes that only one function is parsed at a time. If you could emit part of one function then part of another, then switch back to the original, that would break the type checker.

[dschuff]

I was actually thinking about something like the following:

foo:
  .functype foo (i32) -> ()
  local.get 0

.section .data.bar,"",@
bar:
  .int32 7
  .size bar, 4

.section .text.foo,"",@
  drop
  end_function

It looks like that does actually work today. (if you delete the drop you'll even get the right type check failure.
But still, I'm not sure how valuable it is. I wonder how we'd find out.

BTW, do you have any idea how close we are to being able to round trip some clang test suite through asm rather than directly emitting objects? That would at least tell us whether clang ever does this.

edit: I had wondered whether clang used e.g. .pushsection/.popsection which would make this even easier to write. but it looks like we don't actually even support .pushsection

[dschuff]

This comment was removed by dschuff.

llvm/lib/MC/WasmObjectWriter.cpp

uint32_t WasmObjectWriter::writeCodeSection(const MCAssembler &Asm,
   startSection(Section, wasm::WASM_SEC_CODE);
 
   encodeULEB128(Functions.size(), W->OS);
 
   for (const WasmFunction &Func : Functions) {
     auto *FuncSection = static_cast<MCSectionWasm *>(Func.Section);
 
     int64_t Size = Layout.getSectionAddressSize(FuncSection);
+    // Wasm functions can't have zero size, they at least need to
+    // encode the numer of locals, even if that number is zero.
+    assert(Size);
     encodeULEB128(Size, W->OS);
     FuncSection->setSectionOffset(W->OS.tell() - Section.ContentsOffset);
     Asm.writeSectionData(W->OS, FuncSection, Layout);
   }
 
   // Apply fixups.
   applyRelocations(CodeRelocations, Section.ContentsOffset, Layout);
 

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

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 (WasmSym->isDefined()) {
+      if (WasmSym->isDefined() && CurrentState != FunctionStart) {
         // This .functype indicates a start of a function.
         if (ensureEmptyNestingStack())
           return true;
         CurrentState = FunctionStart;
         LastFunctionLabel = WasmSym;
         push(Function);
       }
       auto Signature = std::make_unique<wasm::WasmSignature>();
bool MatchAndEmitInstruction(SMLoc IDLoc, unsigned & /*Opcode*/,
     }
     }
     llvm_unreachable("Implement any new match types added!");
   }
 
   void doBeforeLabelEmit(MCSymbol *Symbol, SMLoc IDLoc) override {
     // Code below only applies to labels in text sections.
     auto CWS = cast<MCSectionWasm>(getStreamer().getCurrentSection().first);
-    if (!CWS || !CWS->getKind().isText())
+    auto WasmSym = cast<MCSymbolWasm>(Symbol);
+    if (!CWS || !CWS->getKind().isText()) {
+      if (CurrentState == Instructions || CurrentState == FunctionStart ||
+          CurrentState == FunctionLocals) {
+        Parser.Error(IDLoc,
+                     "premature end of function (expected end_function)");
+        CurrentState = EndFunction;
+        pop("end_function", Function);
+      }
       return;
+    }
 
-    auto WasmSym = cast<MCSymbolWasm>(Symbol);
     // Unlike other targets, we don't allow data in text sections (labels
     // declared with .type @object).
     if (WasmSym->getType() == wasm::WASM_SYMBOL_TYPE_DATA) {
       Parser.Error(IDLoc,
                    "Wasm doesn\'t support data symbols in text sections");
       return;
     }
 
+    if (WasmSym->isFunction()) {
+      if (CurrentState == FunctionStart && LastFunctionLabel != WasmSym) {
+        Parser.Error(
+            IDLoc, "previous function not terminated (expected end_function)");
+        pop("end_function", Function);
+      }
+      if (ensureEmptyNestingStack())
+        return;
+      CurrentState = FunctionStart;
+      LastFunctionLabel = WasmSym;
+      push(Function);
+    }
+
     // 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.
 
     // TODO: If the user explicitly creates a new function section, we ignore

llvm/test/MC/WebAssembly/empty-function-body.s

+# RUN: not llvm-mc -triple=wasm32-unknown-unknown -filetype=obj %s -o %t.o 2>&1 | FileCheck %s
+
+# An empty function should produce an error.
+foo:
+  .functype foo () -> ()
+
+# CHECK: empty-function-body.s:8:1: error: Unmatched block construct(s) at function end: function

llvm/test/MC/WebAssembly/empty-function-body2.s

+# RUN: not llvm-mc -triple=wasm32-unknown-unknown -filetype=obj %s -o %t.o 2>&1 | FileCheck %s
+
+# This is the same as empty-function-body.s but with the .functype declaration
+# before the body of the function.
+.functype bar () -> ()
+bar:
+
+# CHECK: empty-function-body2.s:9:1: error: Unmatched block construct(s) at function end: function

llvm/test/MC/WebAssembly/empty-function-body3.s

+# RUN: not llvm-mc -triple=wasm32-unknown-unknown -filetype=obj %s -o %t.o 2>&1 | FileCheck %s
+
+# An empty function should produce an error as soon as a non-function symbol
+# is defined.
+foo:
+  .functype foo () -> ()
+
+.section .data.bar,"",@
+bar:
+# CHECK: empty-function-body3.s:9:1: error: premature end of function (expected end_function)
+  .int32   7
+  .size    bar, 4
+
+.section .text.foo2,"",@
+foo2:
+  .functype foo2 () -> ()
+
+# .. or when another function body is encountered.
+.section .text.bar2,"",@
+.functype bar2 () -> ()
+bar2:
+# CHECK: empty-function-body3.s:21:1: error: previous function not terminated (expected end_function)
+  end_function