[llvm] [WebAssembly] Allow AsmTypeCheck detect multiple errors in function (PR #109705)
Heejin Ahn via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 23 12:00:53 PDT 2024
https://github.com/aheejin created https://github.com/llvm/llvm-project/pull/109705
This allows multiple errors to be reported within a function, rather than returning on the first error and not looking at the rest of the function.
I think the rationale for the previous behavior was that upon encountering the first error, the value stack was not in the correct status anymore and the rest of the function checking was not very meaningful. But this patch makes the instruction push the correct result type upon its completion, so the while the possibility of previous error affecting later instructions is not zero, I think this can be more helpful to assembly hand-writers. This also allows us to write multiple error test cases without creating as many functions.
This is what Wabt and Binaryen wast parsers do as well.
>From 7ad682111a381d6c043401a5a29ebe90d252b86b Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Sat, 21 Sep 2024 03:56:09 +0000
Subject: [PATCH] [WebAssembly] Allow AsmTypeCheck detect multiple errors in
function
This allows multiple errors to be reported within a function, rather
than returning on the first error and not looking at the rest of the
function.
I think the rationale for the previous behavior was that upon
encountering the first error, the value stack was not in the correct
status anymore and the rest of the function checking was not very
meaningful. But this patch makes the instruction push the correct result
type upon its completion, so the while the possibility of previous error
affecting later instructions is not zero, I think this can be more
helpful to assembly hand-writers. This also allows us to write multiple
error test cases without creating as many functions.
This is what Wabt and Binaryen wast parsers do as well.
---
.../AsmParser/WebAssemblyAsmTypeCheck.cpp | 348 ++++++++++--------
.../AsmParser/WebAssemblyAsmTypeCheck.h | 2 -
.../test/MC/WebAssembly/type-checker-errors.s | 22 ++
3 files changed, 214 insertions(+), 158 deletions(-)
diff --git a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
index 9f8f78a5b6adb7..59f3a9f81a3ffb 100644
--- a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
+++ b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
@@ -70,14 +70,9 @@ void WebAssemblyAsmTypeCheck::dumpTypeStack(Twine Msg) {
}
bool WebAssemblyAsmTypeCheck::typeError(SMLoc ErrorLoc, const Twine &Msg) {
- // Once you get one type error in a function, it will likely trigger more
- // which are mostly not helpful.
- if (TypeErrorThisFunction)
- return true;
// If we're currently in unreachable code, we suppress errors completely.
if (Unreachable)
return false;
- TypeErrorThisFunction = true;
dumpTypeStack("current stack: ");
return Parser.Error(ErrorLoc, Msg);
}
@@ -171,11 +166,11 @@ bool WebAssemblyAsmTypeCheck::checkEnd(SMLoc ErrorLoc, bool PopVals) {
bool WebAssemblyAsmTypeCheck::checkSig(SMLoc ErrorLoc,
const wasm::WasmSignature &Sig) {
+ bool Error = false;
for (auto VT : llvm::reverse(Sig.Params))
- if (popType(ErrorLoc, VT))
- return true;
+ Error |= popType(ErrorLoc, VT);
Stack.insert(Stack.end(), Sig.Returns.begin(), Sig.Returns.end());
- return false;
+ return Error;
}
bool WebAssemblyAsmTypeCheck::getSymRef(SMLoc ErrorLoc, const MCOperand &SymOp,
@@ -260,17 +255,16 @@ bool WebAssemblyAsmTypeCheck::getSignature(SMLoc ErrorLoc,
}
bool WebAssemblyAsmTypeCheck::endOfFunction(SMLoc ErrorLoc) {
+ bool Error = false;
// Check the return types.
- for (auto RVT : llvm::reverse(ReturnTypes)) {
- if (popType(ErrorLoc, RVT))
- return true;
- }
+ for (auto RVT : llvm::reverse(ReturnTypes))
+ Error |= popType(ErrorLoc, RVT);
if (!Stack.empty()) {
return typeError(ErrorLoc, std::to_string(Stack.size()) +
" superfluous return values");
}
Unreachable = true;
- return false;
+ return Error;
}
bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
@@ -279,179 +273,221 @@ bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
auto Name = GetMnemonic(Opc);
dumpTypeStack("typechecking " + Name + ": ");
wasm::ValType Type;
+
if (Name == "local.get") {
- if (getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- Stack.push_back(Type);
- } else if (Name == "local.set") {
- if (getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- } else if (Name == "local.tee") {
- if (getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- Stack.push_back(Type);
- } else if (Name == "global.get") {
- if (getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- Stack.push_back(Type);
- } else if (Name == "global.set") {
- if (getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- } else if (Name == "table.get") {
- if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- Stack.push_back(Type);
- } else if (Name == "table.set") {
- if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- } else if (Name == "table.size") {
- if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
+ if (!getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+ Stack.push_back(Type);
+ return false;
+ }
+ return true;
+ }
+
+ if (Name == "local.set") {
+ if (!getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+ return popType(ErrorLoc, Type);
+ return true;
+ }
+
+ if (Name == "local.tee") {
+ if (!getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+ bool Error = popType(ErrorLoc, Type);
+ Stack.push_back(Type);
+ return Error;
+ }
+ return true;
+ }
+
+ if (Name == "global.get") {
+ if (!getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+ Stack.push_back(Type);
+ return false;
+ }
+ return true;
+ }
+
+ if (Name == "global.set") {
+ if (!getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+ return popType(ErrorLoc, Type);
+ return true;
+ }
+
+ if (Name == "table.get") {
+ bool Error = popType(ErrorLoc, wasm::ValType::I32);
+ if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+ Stack.push_back(Type);
+ return Error;
+ }
+ return true;
+ }
+
+ if (Name == "table.set") {
+ bool Error = false;
+ if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+ Error |= popType(ErrorLoc, Type);
+ else
+ Error = true;
+ Error |= popType(ErrorLoc, wasm::ValType::I32);
+ return Error;
+ }
+
+ if (Name == "table.size") {
+ bool Error = getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type);
Stack.push_back(wasm::ValType::I32);
- } else if (Name == "table.grow") {
- if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
+ return Error;
+ }
+
+ if (Name == "table.grow") {
+ bool Error = popType(ErrorLoc, wasm::ValType::I32);
+ if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+ Error |= popType(ErrorLoc, Type);
+ else
+ Error = true;
Stack.push_back(wasm::ValType::I32);
- } else if (Name == "table.fill") {
- if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
- return true;
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- } else if (Name == "memory.fill") {
+ return Error;
+ }
+
+ if (Name == "table.fill") {
+ bool Error = popType(ErrorLoc, wasm::ValType::I32);
+ if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+ Error |= popType(ErrorLoc, Type);
+ else
+ Error = true;
+ Error |= popType(ErrorLoc, wasm::ValType::I32);
+ return Error;
+ }
+
+ if (Name == "memory.fill") {
Type = is64 ? wasm::ValType::I64 : wasm::ValType::I32;
- if (popType(ErrorLoc, Type))
- return true;
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- } else if (Name == "memory.copy") {
+ bool Error = popType(ErrorLoc, Type);
+ Error |= popType(ErrorLoc, wasm::ValType::I32);
+ Error |= popType(ErrorLoc, Type);
+ return Error;
+ }
+
+ if (Name == "memory.copy") {
Type = is64 ? wasm::ValType::I64 : wasm::ValType::I32;
- if (popType(ErrorLoc, Type))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- } else if (Name == "memory.init") {
+ bool Error = popType(ErrorLoc, Type);
+ Error |= popType(ErrorLoc, Type);
+ Error |= popType(ErrorLoc, Type);
+ return Error;
+ }
+
+ if (Name == "memory.init") {
Type = is64 ? wasm::ValType::I64 : wasm::ValType::I32;
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- if (popType(ErrorLoc, Type))
- return true;
- } else if (Name == "drop") {
- if (popType(ErrorLoc, {}))
- return true;
- } else if (Name == "try" || Name == "block" || Name == "loop" ||
- Name == "if") {
- if (Name == "if" && popType(ErrorLoc, wasm::ValType::I32))
- return true;
+ bool Error = popType(ErrorLoc, wasm::ValType::I32);
+ Error |= popType(ErrorLoc, wasm::ValType::I32);
+ Error |= popType(ErrorLoc, Type);
+ return Error;
+ }
+
+ if (Name == "drop") {
+ return popType(ErrorLoc, {});
+ }
+
+ if (Name == "try" || Name == "block" || Name == "loop" || Name == "if") {
if (Name == "loop")
BrStack.emplace_back(LastSig.Params.begin(), LastSig.Params.end());
else
BrStack.emplace_back(LastSig.Returns.begin(), LastSig.Returns.end());
- } else if (Name == "end_block" || Name == "end_loop" || Name == "end_if" ||
- Name == "else" || Name == "end_try" || Name == "catch" ||
- Name == "catch_all" || Name == "delegate") {
- if (checkEnd(ErrorLoc,
- Name == "else" || Name == "catch" || Name == "catch_all"))
+ if (Name == "if" && popType(ErrorLoc, wasm::ValType::I32))
return true;
+ return false;
+ }
+
+ if (Name == "end_block" || Name == "end_loop" || Name == "end_if" ||
+ Name == "else" || Name == "end_try" || Name == "catch" ||
+ Name == "catch_all" || Name == "delegate") {
+ bool Error = checkEnd(ErrorLoc, Name == "else" || Name == "catch" ||
+ Name == "catch_all");
Unreachable = false;
if (Name == "catch") {
const wasm::WasmSignature *Sig = nullptr;
- if (getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
- wasm::WASM_SYMBOL_TYPE_TAG, Sig))
- return true;
- // catch instruction pushes values whose types are specified in the tag's
- // "params" part
- Stack.insert(Stack.end(), Sig->Params.begin(), Sig->Params.end());
+ if (!getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
+ wasm::WASM_SYMBOL_TYPE_TAG, Sig))
+ // catch instruction pushes values whose types are specified in the
+ // tag's "params" part
+ Stack.insert(Stack.end(), Sig->Params.begin(), Sig->Params.end());
+ else
+ Error = true;
}
- } else if (Name == "br") {
+ return Error;
+ }
+
+ if (Name == "br") {
const MCOperand &Operand = Inst.getOperand(0);
if (!Operand.isImm())
- return false;
- if (checkBr(ErrorLoc, static_cast<size_t>(Operand.getImm())))
- return true;
- } else if (Name == "return") {
- if (endOfFunction(ErrorLoc))
return true;
- } else if (Name == "call_indirect" || Name == "return_call_indirect") {
+ return checkBr(ErrorLoc, static_cast<size_t>(Operand.getImm()));
+ }
+
+ if (Name == "return") {
+ return endOfFunction(ErrorLoc);
+ }
+
+ if (Name == "call_indirect" || Name == "return_call_indirect") {
// Function value.
- if (popType(ErrorLoc, wasm::ValType::I32))
- return true;
- if (checkSig(ErrorLoc, LastSig))
- return true;
+ bool Error = popType(ErrorLoc, wasm::ValType::I32);
+ Error |= checkSig(ErrorLoc, LastSig);
if (Name == "return_call_indirect" && endOfFunction(ErrorLoc))
return true;
- } else if (Name == "call" || Name == "return_call") {
+ return Error;
+ }
+
+ if (Name == "call" || Name == "return_call") {
+ bool Error = false;
const wasm::WasmSignature *Sig = nullptr;
- if (getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
- wasm::WASM_SYMBOL_TYPE_FUNCTION, Sig))
- return true;
- if (checkSig(ErrorLoc, *Sig))
- return true;
+ if (!getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
+ wasm::WASM_SYMBOL_TYPE_FUNCTION, Sig))
+ Error |= checkSig(ErrorLoc, *Sig);
+ else
+ Error = true;
if (Name == "return_call" && endOfFunction(ErrorLoc))
return true;
- } else if (Name == "unreachable") {
+ return Error;
+ }
+
+ if (Name == "unreachable") {
Unreachable = true;
- } else if (Name == "ref.is_null") {
- if (popRefType(ErrorLoc))
- return true;
+ return false;
+ }
+
+ if (Name == "ref.is_null") {
+ bool Error = popRefType(ErrorLoc);
Stack.push_back(wasm::ValType::I32);
- } else if (Name == "throw") {
+ return Error;
+ }
+
+ if (Name == "throw") {
const wasm::WasmSignature *Sig = nullptr;
- if (getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
- wasm::WASM_SYMBOL_TYPE_TAG, Sig))
- return true;
- if (checkSig(ErrorLoc, *Sig))
- return true;
- } else {
- // The current instruction is a stack instruction which doesn't have
- // explicit operands that indicate push/pop types, so we get those from
- // the register version of the same instruction.
- auto RegOpc = WebAssembly::getRegisterOpcode(Opc);
- assert(RegOpc != -1 && "Failed to get register version of MC instruction");
- const auto &II = MII.get(RegOpc);
- // First pop all the uses off the stack and check them.
- for (unsigned I = II.getNumOperands(); I > II.getNumDefs(); I--) {
- const auto &Op = II.operands()[I - 1];
- if (Op.OperandType == MCOI::OPERAND_REGISTER) {
- auto VT = WebAssembly::regClassToValType(Op.RegClass);
- if (popType(ErrorLoc, VT))
- return true;
- }
- }
- // Now push all the defs onto the stack.
- for (unsigned I = 0; I < II.getNumDefs(); I++) {
- const auto &Op = II.operands()[I];
- assert(Op.OperandType == MCOI::OPERAND_REGISTER && "Register expected");
+ if (!getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
+ wasm::WASM_SYMBOL_TYPE_TAG, Sig))
+ return checkSig(ErrorLoc, *Sig);
+ return true;
+ }
+
+ // The current instruction is a stack instruction which doesn't have
+ // explicit operands that indicate push/pop types, so we get those from
+ // the register version of the same instruction.
+ auto RegOpc = WebAssembly::getRegisterOpcode(Opc);
+ assert(RegOpc != -1 && "Failed to get register version of MC instruction");
+ const auto &II = MII.get(RegOpc);
+ bool Error = false;
+ // First pop all the uses off the stack and check them.
+ for (unsigned I = II.getNumOperands(); I > II.getNumDefs(); I--) {
+ const auto &Op = II.operands()[I - 1];
+ if (Op.OperandType == MCOI::OPERAND_REGISTER) {
auto VT = WebAssembly::regClassToValType(Op.RegClass);
- Stack.push_back(VT);
+ Error |= popType(ErrorLoc, VT);
}
}
- return false;
+ // Now push all the defs onto the stack.
+ for (unsigned I = 0; I < II.getNumDefs(); I++) {
+ const auto &Op = II.operands()[I];
+ assert(Op.OperandType == MCOI::OPERAND_REGISTER && "Register expected");
+ auto VT = WebAssembly::regClassToValType(Op.RegClass);
+ Stack.push_back(VT);
+ }
+ return Error;
}
} // end namespace llvm
diff --git a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
index 9ba5693719e91a..4020234e2432ff 100644
--- a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
+++ b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
@@ -33,7 +33,6 @@ class WebAssemblyAsmTypeCheck final {
SmallVector<wasm::ValType, 16> LocalTypes;
SmallVector<wasm::ValType, 4> ReturnTypes;
wasm::WasmSignature LastSig;
- bool TypeErrorThisFunction = false;
bool Unreachable = false;
bool is64;
@@ -68,7 +67,6 @@ class WebAssemblyAsmTypeCheck final {
BrStack.clear();
LocalTypes.clear();
ReturnTypes.clear();
- TypeErrorThisFunction = false;
Unreachable = false;
}
};
diff --git a/llvm/test/MC/WebAssembly/type-checker-errors.s b/llvm/test/MC/WebAssembly/type-checker-errors.s
index e8b8274036a832..3106fe76c8449f 100644
--- a/llvm/test/MC/WebAssembly/type-checker-errors.s
+++ b/llvm/test/MC/WebAssembly/type-checker-errors.s
@@ -93,12 +93,14 @@ global_set_type_mismatch:
table_get_expected_expression_operand:
.functype table_get_expected_expression_operand () -> ()
+ i32.const 0
# CHECK: :[[@LINE+1]]:13: error: expected expression operand
table.get 1
end_function
table_get_missing_tabletype:
.functype table_get_missing_tabletype () -> ()
+ i32.const 0
# CHECK: :[[@LINE+1]]:13: error: symbol foo: missing .tabletype
table.get foo
end_function
@@ -851,3 +853,23 @@ br_incorrect_func_signature:
drop
i32.const 1
end_function
+
+multiple_errors_in_function:
+ .functype multiple_errors_in_function () -> ()
+# CHECK: :[[@LINE+2]]:3: error: empty stack while popping i32
+# CHECK: :[[@LINE+1]]:13: error: expected expression operand
+ table.get 1
+
+# CHECK: :[[@LINE+3]]:3: error: empty stack while popping i32
+# CHECK: :[[@LINE+2]]:3: error: empty stack while popping externref
+# CHECK: :[[@LINE+1]]:3: error: empty stack while popping i32
+ table.fill valid_table
+
+ f32.const 0.0
+ ref.null_extern
+# CHECK: :[[@LINE+2]]:3: error: popped externref, expected i32
+# CHECK: :[[@LINE+1]]:3: error: popped f32, expected i32
+ i32.add
+ drop
+
+ end_function
More information about the llvm-commits
mailing list