Skip to content

[WebAssembly] Use 'any' type in more cases in AsmTypeCheck #110403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 30, 2024
Merged

[WebAssembly] Use 'any' type in more cases in AsmTypeCheck #110403

merged 3 commits into from
Sep 30, 2024

Conversation

@aheejin
Copy link
Member

@aheejin aheejin commented Sep 29, 2024

Now that we support 'any' type in the value stack in the checker, this uses it in more places.

When an instruction pops multiple values, rather than popping in one by one and generating multiple error messages, it adds them to a vector and pops them at once. When the type to be popped is not clear, it pops 'any', at least makes sure there are correct number of values on the stack. So for example, in case of table.fill, which expects [i32 t i32] (where t is the type of the elements in the table), it pops them at once, generating an error message like

error: type mismatch, expected [i32, externref, i32] but got [...]

In case the table is invalid so we don't know the type, it tries to pop an 'any' instead, popping whatever value there is:

error: type mismatch, expected [i32, any, i32] but got [...]

Checks done on other instructions based on the register info are already popping and pushing types in vectors, after #110094:

llvm-project/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp

Lines 515 to 536 in a522516

// 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.
SmallVector<wasm::ValType, 4> PopTypes;
for (unsigned I = II.getNumDefs(); I < II.getNumOperands(); I++) {
const auto &Op = II.operands()[I];
if (Op.OperandType == MCOI::OPERAND_REGISTER)
PopTypes.push_back(WebAssembly::regClassToValType(Op.RegClass));
}
bool Error = checkAndPopTypes(ErrorLoc, PopTypes, false);
SmallVector<wasm::ValType, 4> PushTypes;
// 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");
PushTypes.push_back(WebAssembly::regClassToValType(Op.RegClass));
}
pushTypes(PushTypes);

This also pushes 'any' in case the type to push is unclear. For example, local/global.set pushes a value of the type specified in the local or global, but in case that local or global is invalid, we push 'any' instead, which will match with whatever type.

The objective of all these is not to make one instruction's error propragate continuously into subsequent instructions. This also matches Wabt's behavior.

This also renames checkAndPopTypes to just popTypes, to be consistent with a single-element version popType. popType(s) also does type checks.

@aheejin
[WebAssembly] Use 'any' type in more cases in AsmTypeCheck
a949136 
Now that we support 'any' type in the value stack in the checker, this
uses it in more places.

When an instruction pops multiple values, rather than popping in one by
one and generating multiple error messages, it adds them to a vector and
pops them at once. When the type to be popped is not clear, it pops
'any', at least makes sure there are correct number of values on the
stack. So for example, in case of `table.fill`, which expects `[i32 t
i32]` (where t is the type of the elements in the table), it pops them
at once, generating an error message like
```console
error: type mismatch, expected [i32, externref, i32] but got [...]
```
In case the table is invalid so we don't know the type, it tries to pop
an 'any' instead, popping whatever value there is:
```console
error: type mismatch, expected [i32, any, i32] but got [...]
```

Checks done on other instructions based on the register info are already
popping and pushing types in vectors, after llvm#110094:
https://github.com/llvm/llvm-project/blob/a52251675f001115b225f57362d37e92b7355ef9/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp#L515-L536

This also pushes 'any' in case the type to push is unclear. For example,
`local/global.set` pushes a value of the type specified in the local or
global, but in case that local or global is invalid, we push 'any'
instead, which will match with whatever type.

The objective of all these is not to make one instruction's error
propragate continuously into subsequent instructions. This also matches
Wabt's behavior.

This also renames `checkAndPopTypes` to just `popTypes`, to be
consistent with a single-element version `popType`. `popType(s)` also
does type checks.
@aheejin aheejin requested a review from dschuff September 29, 2024 02:38
@llvmbot llvmbot added backend:WebAssembly llvm:mc Machine (object) code labels Sep 29, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 29, 2024 •
edited
Loading

@llvm/pr-subscribers-backend-webassembly

@llvm/pr-subscribers-mc

Author: Heejin Ahn (aheejin)

Changes

Now that we support 'any' type in the value stack in the checker, this uses it in more places.

When an instruction pops multiple values, rather than popping in one by one and generating multiple error messages, it adds them to a vector and pops them at once. When the type to be popped is not clear, it pops 'any', at least makes sure there are correct number of values on the stack. So for example, in case of table.fill, which expects [i32 t i32] (where t is the type of the elements in the table), it pops them at once, generating an error message like

error: type mismatch, expected [i32, externref, i32] but got [...]

In case the table is invalid so we don't know the type, it tries to pop an 'any' instead, popping whatever value there is:

error: type mismatch, expected [i32, any, i32] but got [...]

Checks done on other instructions based on the register info are already popping and pushing types in vectors, after #110094:

llvm-project/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp

Lines 515 to 536 in a522516

// 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.
SmallVector<wasm::ValType, 4> PopTypes;
for (unsigned I = II.getNumDefs(); I < II.getNumOperands(); I++) {
const auto &Op = II.operands()[I];
if (Op.OperandType == MCOI::OPERAND_REGISTER)
PopTypes.push_back(WebAssembly::regClassToValType(Op.RegClass));
}
bool Error = checkAndPopTypes(ErrorLoc, PopTypes, false);
SmallVector<wasm::ValType, 4> PushTypes;
// 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");
PushTypes.push_back(WebAssembly::regClassToValType(Op.RegClass));
}
pushTypes(PushTypes);

This also pushes 'any' in case the type to push is unclear. For example, local/global.set pushes a value of the type specified in the local or global, but in case that local or global is invalid, we push 'any' instead, which will match with whatever type.

The objective of all these is not to make one instruction's error propragate continuously into subsequent instructions. This also matches Wabt's behavior.

This also renames checkAndPopTypes to just popTypes, to be consistent with a single-element version popType. popType(s) also does type checks.

Full diff: https://github.com/llvm/llvm-project/pull/110403.diff

3 Files Affected:

  • (modified) llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp (+49-27)
  • (modified) llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h (+10-6)
  • (modified) llvm/test/MC/WebAssembly/type-checker-errors.s (+40-20)
diff --git a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
index 845bf3976c22b5..a0a49ee6377803 100644
--- a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
+++ b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
@@ -19,6 +19,7 @@
 #include "MCTargetDesc/WebAssemblyTargetStreamer.h"
 #include "TargetInfo/WebAssemblyTargetInfo.h"
 #include "WebAssembly.h"
+#include "llvm/BinaryFormat/Wasm.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
@@ -159,15 +160,15 @@ bool WebAssemblyAsmTypeCheck::checkTypes(SMLoc ErrorLoc,
                                  getTypesString(Stack, StackStartPos));
 }
 
-bool WebAssemblyAsmTypeCheck::checkAndPopTypes(SMLoc ErrorLoc,
-                                               ArrayRef<wasm::ValType> ValTypes,
-                                               bool ExactMatch) {
-  return checkAndPopTypes(ErrorLoc, valTypeToStackType(ValTypes), ExactMatch);
+bool WebAssemblyAsmTypeCheck::popTypes(SMLoc ErrorLoc,
+                                       ArrayRef<wasm::ValType> ValTypes,
+                                       bool ExactMatch) {
+  return popTypes(ErrorLoc, valTypeToStackType(ValTypes), ExactMatch);
 }
 
-bool WebAssemblyAsmTypeCheck::checkAndPopTypes(SMLoc ErrorLoc,
-                                               ArrayRef<StackType> Types,
-                                               bool ExactMatch) {
+bool WebAssemblyAsmTypeCheck::popTypes(SMLoc ErrorLoc,
+                                       ArrayRef<StackType> Types,
+                                       bool ExactMatch) {
   bool Error = checkTypes(ErrorLoc, Types, ExactMatch);
   auto NumPops = std::min(Stack.size(), Types.size());
   for (size_t I = 0, E = NumPops; I != E; I++)
@@ -176,7 +177,7 @@ bool WebAssemblyAsmTypeCheck::checkAndPopTypes(SMLoc ErrorLoc,
 }
 
 bool WebAssemblyAsmTypeCheck::popType(SMLoc ErrorLoc, StackType Type) {
-  return checkAndPopTypes(ErrorLoc, {Type}, false);
+  return popTypes(ErrorLoc, {Type});
 }
 
 bool WebAssemblyAsmTypeCheck::popRefType(SMLoc ErrorLoc) {
@@ -207,7 +208,7 @@ bool WebAssemblyAsmTypeCheck::checkBr(SMLoc ErrorLoc, size_t Level) {
                      StringRef("br: invalid depth ") + std::to_string(Level));
   const SmallVector<wasm::ValType, 4> &Expected =
       BrStack[BrStack.size() - Level - 1];
-  return checkTypes(ErrorLoc, Expected, false);
+  return checkTypes(ErrorLoc, Expected);
   return false;
 }
 
@@ -216,13 +217,13 @@ bool WebAssemblyAsmTypeCheck::checkEnd(SMLoc ErrorLoc, bool PopVals) {
     BrStack.pop_back();
 
   if (PopVals)
-    return checkAndPopTypes(ErrorLoc, LastSig.Returns, false);
-  return checkTypes(ErrorLoc, LastSig.Returns, false);
+    return popTypes(ErrorLoc, LastSig.Returns);
+  return checkTypes(ErrorLoc, LastSig.Returns);
 }
 
 bool WebAssemblyAsmTypeCheck::checkSig(SMLoc ErrorLoc,
                                        const wasm::WasmSignature &Sig) {
-  bool Error = checkAndPopTypes(ErrorLoc, Sig.Params, false);
+  bool Error = popTypes(ErrorLoc, Sig.Params);
   pushTypes(Sig.Returns);
   return Error;
 }
@@ -309,7 +310,7 @@ bool WebAssemblyAsmTypeCheck::getSignature(SMLoc ErrorLoc,
 }
 
 bool WebAssemblyAsmTypeCheck::endOfFunction(SMLoc ErrorLoc, bool ExactMatch) {
-  bool Error = checkAndPopTypes(ErrorLoc, ReturnTypes, ExactMatch);
+  bool Error = popTypes(ErrorLoc, ReturnTypes, ExactMatch);
   Unreachable = true;
   return Error;
 }
@@ -326,12 +327,14 @@ bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
       pushType(Type);
       return false;
     }
+    pushType(Any{});
     return true;
   }
 
   if (Name == "local.set") {
     if (!getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
       return popType(ErrorLoc, Type);
+    popType(ErrorLoc, Any{});
     return true;
   }
 
@@ -341,6 +344,8 @@ bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
       pushType(Type);
       return Error;
     }
+    popType(ErrorLoc, Any{});
+    pushType(Any{});
     return true;
   }
 
@@ -349,12 +354,14 @@ bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
       pushType(Type);
       return false;
     }
+    pushType(Any{});
     return true;
   }
 
   if (Name == "global.set") {
     if (!getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
       return popType(ErrorLoc, Type);
+    popType(ErrorLoc, Any{});
     return true;
   }
 
@@ -364,16 +371,21 @@ bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
       pushType(Type);
       return Error;
     }
+    pushType(Any{});
     return true;
   }
 
   if (Name == "table.set") {
     bool Error = false;
-    if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      Error |= popType(ErrorLoc, Type);
-    else
+    SmallVector<StackType, 2> PopTypes;
+    PopTypes.push_back(wasm::ValType::I32);
+    if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+      PopTypes.push_back(Type);
+    } else {
       Error = true;
-    Error |= popType(ErrorLoc, wasm::ValType::I32);
+      PopTypes.push_back(Any{});
+    }
+    Error |= popTypes(ErrorLoc, PopTypes);
     return Error;
   }
 
@@ -384,22 +396,32 @@ bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
   }
 
   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
+    bool Error = false;
+    SmallVector<StackType, 2> PopTypes;
+    if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+      PopTypes.push_back(Type);
+    } else {
       Error = true;
+      PopTypes.push_back(Any{});
+    }
+    PopTypes.push_back(wasm::ValType::I32);
+    Error |= popTypes(ErrorLoc, PopTypes);
     pushType(wasm::ValType::I32);
     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
+    bool Error = false;
+    SmallVector<StackType, 2> PopTypes;
+    PopTypes.push_back(wasm::ValType::I32);
+    if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+      PopTypes.push_back(Type);
+    } else {
       Error = true;
-    Error |= popType(ErrorLoc, wasm::ValType::I32);
+      PopTypes.push_back(Any{});
+    }
+    PopTypes.push_back(wasm::ValType::I32);
+    Error |= popTypes(ErrorLoc, PopTypes);
     return Error;
   }
 
@@ -525,7 +547,7 @@ bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
     if (Op.OperandType == MCOI::OPERAND_REGISTER)
       PopTypes.push_back(WebAssembly::regClassToValType(Op.RegClass));
   }
-  bool Error = checkAndPopTypes(ErrorLoc, PopTypes, false);
+  bool Error = popTypes(ErrorLoc, PopTypes);
   SmallVector<wasm::ValType, 4> PushTypes;
   // Now push all the defs onto the stack.
   for (unsigned I = 0; I < II.getNumDefs(); I++) {
diff --git a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
index 9fd35a26f30e50..df063d749e3b40 100644
--- a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
+++ b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
@@ -40,17 +40,21 @@ class WebAssemblyAsmTypeCheck final {
   bool Unreachable = false;
   bool Is64;
 
+  // checkTypes checks 'Types' against the value stack. popTypes checks 'Types'
+  // against the value stack and also pops them.
+  //
   // If ExactMatch is true, 'Types' will be compared against not only the top of
   // the value stack but the whole remaining value stack
   // (TODO: This should be the whole remaining value stack "at the the current
   // block level", which has not been implemented yet)
   bool checkTypes(SMLoc ErrorLoc, ArrayRef<wasm::ValType> Types,
-                  bool ExactMatch);
-  bool checkTypes(SMLoc ErrorLoc, ArrayRef<StackType> Types, bool ExactMatch);
-  bool checkAndPopTypes(SMLoc ErrorLoc, ArrayRef<wasm::ValType> Types,
-                        bool ExactMatch);
-  bool checkAndPopTypes(SMLoc ErrorLoc, ArrayRef<StackType> Types,
-                        bool ExactMatch);
+                  bool ExactMatch = false);
+  bool checkTypes(SMLoc ErrorLoc, ArrayRef<StackType> Types,
+                  bool ExactMatch = false);
+  bool popTypes(SMLoc ErrorLoc, ArrayRef<wasm::ValType> Types,
+                bool ExactMatch = false);
+  bool popTypes(SMLoc ErrorLoc, ArrayRef<StackType> Types,
+                bool ExactMatch = false);
   bool popType(SMLoc ErrorLoc, StackType Type);
   bool popRefType(SMLoc ErrorLoc);
   bool popAnyType(SMLoc ErrorLoc);
diff --git a/llvm/test/MC/WebAssembly/type-checker-errors.s b/llvm/test/MC/WebAssembly/type-checker-errors.s
index 5fdc2f56daf57b..d81c5aff0a7e92 100644
--- a/llvm/test/MC/WebAssembly/type-checker-errors.s
+++ b/llvm/test/MC/WebAssembly/type-checker-errors.s
@@ -139,15 +139,14 @@ table_set_missing_tabletype:
 
 table_set_empty_stack_while_popping_1:
   .functype table_set_empty_stack_while_popping_1 () -> ()
-# CHECK: :[[@LINE+2]]:3: error: type mismatch, expected [externref] but got []
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got []
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref] but got []
   table.set valid_table
   end_function
 
 table_set_empty_stack_while_popping_2:
   .functype table_set_empty_stack_while_popping_2 (externref) -> ()
   local.get 0
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got []
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref] but got [externref]
   table.set valid_table
   end_function
 
@@ -155,7 +154,7 @@ table_set_type_mismatch_1:
   .functype table_set_type_mismatch_1 () -> ()
   i32.const 0
   ref.null_func
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [externref] but got [funcref]
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref] but got [i32, funcref]
   table.set valid_table
   end_function
 
@@ -163,7 +162,7 @@ table_set_type_mismatch_2:
   .functype table_set_type_mismatch_2 () -> ()
   f32.const 1.0
   ref.null_extern
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got [f32]
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref] but got [f32, externref]
   table.set valid_table
   end_function
 
@@ -187,17 +186,14 @@ table_fill_missing_tabletype:
 
 table_fill_empty_stack_while_popping_1:
   .functype table_fill_empty_stack_while_popping_1 () -> ()
-# CHECK: :[[@LINE+3]]:3: error: type mismatch, expected [i32] but got []
-# CHECK: :[[@LINE+2]]:3: error: type mismatch, expected [externref] but got []
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got []
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref, i32] but got []
   table.fill valid_table
   end_function
 
 table_fill_empty_stack_while_popping_2:
   .functype table_fill_empty_stack_while_popping_2 (i32) -> ()
   local.get 0
-# CHECK: :[[@LINE+2]]:3: error: type mismatch, expected [externref] but got []
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got []
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref, i32] but got [i32]
   table.fill valid_table
   end_function
 
@@ -205,7 +201,7 @@ table_fill_empty_stack_while_popping_3:
   .functype table_fill_empty_stack_while_popping_3 (i32, externref) -> ()
   local.get 1
   local.get 0
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got []
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref, i32] but got [externref, i32]
   table.fill valid_table
   end_function
 
@@ -214,7 +210,7 @@ table_fill_type_mismatch_1:
   i32.const 0
   ref.null_extern
   ref.null_func
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got [funcref]
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref, i32] but got [i32, externref, funcref]
   table.fill valid_table
   end_function
 
@@ -223,7 +219,7 @@ table_fill_type_mismatch_2:
   i32.const 0
   ref.null_func
   i32.const 1
-# CHECK: [[@LINE+1]]:3: error: type mismatch, expected [externref] but got [funcref]
+# CHECK: [[@LINE+1]]:3: error: type mismatch, expected [i32, externref, i32] but got [i32, funcref, i32]
   table.fill valid_table
   end_function
 
@@ -232,7 +228,7 @@ table_fill_type_mismatch_3:
   f32.const 2.0
   ref.null_extern
   i32.const 1
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got [f32]
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref, i32] but got [f32, externref, i32]
   table.fill valid_table
   end_function
 
@@ -241,7 +237,7 @@ table_fill_type_mismatch_4:
   i32.const 1
   ref.null_exn
   i32.const 1
-# CHECK: [[@LINE+1]]:3: error: type mismatch, expected [externref] but got [exnref]
+# CHECK: [[@LINE+1]]:3: error: type mismatch, expected [i32, externref, i32] but got [i32, exnref, i32]
   table.fill valid_table
   end_function
 
@@ -256,7 +252,7 @@ table_grow_non_exist_table:
 table_grow_type_mismatch_1:
   .functype table_grow_type_mismatch_1 (externref, i32) -> (i32)
   local.get 1
-# CHECK: [[@LINE+1]]:3: error: type mismatch, expected [externref] but got []
+# CHECK: [[@LINE+1]]:3: error: type mismatch, expected [externref, i32] but got [i32]
   table.grow valid_table
   end_function
 
@@ -264,7 +260,7 @@ table_grow_type_mismatch_2:
   .functype table_grow_type_mismatch_2 (externref, i32) -> (i32)
   local.get 0
   local.get 0
-# CHECK: [[@LINE+1]]:3: error: type mismatch, expected [i32] but got [externref]
+# CHECK: [[@LINE+1]]:3: error: type mismatch, expected [externref, i32] but got [externref, externref]
   table.grow valid_table
   end_function
 
@@ -883,9 +879,7 @@ multiple_errors_in_function:
 # CHECK: :[[@LINE+1]]:13: error: expected expression operand
   table.get 1
 
-# CHECK: :[[@LINE+3]]:3: error: type mismatch, expected [i32] but got []
-# CHECK: :[[@LINE+2]]:3: error: type mismatch, expected [externref] but got []
-# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got []
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32, externref, i32] but got [any]
   table.fill valid_table
 
   f32.const 0.0
@@ -905,3 +899,29 @@ call_with_multi_param_and_return:
   call take_and_return_multi
 # CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [i32] but got [i32, i64, f32, f64]
   end_function
+
+.functype callee (f32, i32) -> ()
+
+any_value_on_stack:
+  .functype any_value_on_stack () -> ()
+  # This local does not exist so it should error out, but it should put an 'any'
+  # value on the stack so 'call callee' should not error out again
+# CHECK: :[[@LINE+1]]:13: error: no local type specified for index 0
+  local.get 0
+  i32.const 0
+# CHECK-NOT: :[[@LINE+1]]:3: error: type mismatch
+  call callee
+
+  # But this time 'call callee' should error out
+  i32.const 0
+# CHECK: :[[@LINE+1]]:13: error: no local type specified for index 0
+  local.get 0
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [f32, i32] but got [i32, any]
+  call callee
+
+# CHECK: :[[@LINE+2]]:13: error: no local type specified for index 0
+# CHECK: :[[@LINE+1]]:3: error: type mismatch, expected [any] but got []
+  local.set 0
+  drop
+
+  end_function

aheejin
aheejin commented Sep 29, 2024
View reviewed changes
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
Comment on lines +380 to +387
SmallVector<StackType, 2> PopTypes;
PopTypes.push_back(wasm::ValType::I32);
if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
PopTypes.push_back(Type);
} else {
Error = true;
Error |= popType(ErrorLoc, wasm::ValType::I32);
PopTypes.push_back(Any{});
}
Copy link
Member Author

@aheejin aheejin Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that given that we are now passing a vector of types, the order of types should be in reverse, because the types in the vector will be popped from the end.

@aheejin
Copy link
Member Author

aheejin commented Sep 30, 2024

The CI failures are irrelevant. Merging.

@aheejin aheejin merged commit 7b23468 into llvm:main Sep 30, 2024
4 checks passed
@aheejin aheejin deleted the typecheck_any branch September 30, 2024 22:37
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
@aheejin @Sterling-Augustine
[WebAssembly] Use 'any' type in more cases in AsmTypeCheck (llvm#110403)
9690d5c 
Now that we support 'any' type in the value stack in the checker, this
uses it in more places.

When an instruction pops multiple values, rather than popping in one by
one and generating multiple error messages, it adds them to a vector and
pops them at once. When the type to be popped is not clear, it pops
'any', at least makes sure there are correct number of values on the
stack. So for example, in case of `table.fill`, which expects `[i32 t
i32]` (where t is the type of the elements in the table), it pops them
at once, generating an error message like
```console
error: type mismatch, expected [i32, externref, i32] but got [...]
```
In case the table is invalid so we don't know the type, it tries to pop
an 'any' instead, popping whatever value there is:
```console
error: type mismatch, expected [i32, any, i32] but got [...]
```

Checks done on other instructions based on the register info are already
popping and pushing types in vectors, after llvm#110094:
https://github.com/llvm/llvm-project/blob/a52251675f001115b225f57362d37e92b7355ef9/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp#L515-L536

This also pushes 'any' in case the type to push is unclear. For example,
`local/global.set` pushes a value of the type specified in the local or
global, but in case that local or global is invalid, we push 'any'
instead, which will match with whatever type.

The objective of all these is not to make one instruction's error
propragate continuously into subsequent instructions. This also matches
Wabt's behavior.

This also renames `checkAndPopTypes` to just `popTypes`, to be
consistent with a single-element version `popType`. `popType(s)` also
does type checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dschuff dschuff dschuff approved these changes

Assignees

No one assigned

Labels

backend:WebAssembly llvm:mc Machine (object) code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aheejin @llvmbot @dschuff