Skip to content
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.

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

[WebAssembly] Print instructions with type checking errors #111067

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Oct 3, 2024

When there was a type checking error, we didn't run InstPrinter. This can be confusing because when there is an error in, say, block parameter type, InstPrinter doesn't run even if it has nothing to do with block parameter types, and all those updates to ControlFlowStack or TryStack do not happen:

case WebAssembly::LOOP:
case WebAssembly::LOOP_S:
printAnnotation(OS, "label" + utostr(ControlFlowCounter) + ':');
ControlFlowStack.push_back(std::make_pair(ControlFlowCounter++, true));
return;
case WebAssembly::BLOCK:
case WebAssembly::BLOCK_S:
ControlFlowStack.push_back(std::make_pair(ControlFlowCounter++, false));
return;
case WebAssembly::TRY:
case WebAssembly::TRY_S:
ControlFlowStack.push_back(std::make_pair(ControlFlowCounter, false));
TryStack.push_back(ControlFlowCounter++);
EHInstStack.push_back(TRY);
return;

For example,

block (i32) -> () ;; Block input parameter error
end_block         ;; Now this errors out as "End marker mismatch"

This is confusing because there is a block and the end_block is not a mismatch. Only that block has a type checking error, but that's not an end marker mismatch.

I think we can just print the instruction whether we had a type checking error or not, and this will be less confusing.

When there was a type checking error, we didn't run `InstPrinter`. This
can be confusing because when there is an error in, say, block parameter
type, `InstPrinter` doesn't run even if it has nothing to do with block
parameter types, and all those updates to `ControlFlowStack` or
`TryStack` do not happen:
https://github.com/llvm/llvm-project/blob/c20b90ab8557b38efe8e8e993d41d8c08b798267/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp#L135-L151

For example,
```wast
block (i32) -> () ;; Block input parameter error
end_block         ;; Now this errors out as "End marker mismatch"
```
This is confusing because there is a `block` and the `end_block` is not
a mismatch. Only that `block` has a type checking error, but that's not
an end marker mismatch.

I think we can just print the instruction whether we had a type checking
error or not, and this will be less confusing.
@aheejin aheejin requested a review from dschuff October 3, 2024 22:21
@llvmbot llvmbot added backend:WebAssembly mc Machine (object) code labels Oct 3, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2024

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-webassembly

Author: Heejin Ahn (aheejin)

Changes

When there was a type checking error, we didn't run InstPrinter. This can be confusing because when there is an error in, say, block parameter type, InstPrinter doesn't run even if it has nothing to do with block parameter types, and all those updates to ControlFlowStack or TryStack do not happen:

case WebAssembly::LOOP:
case WebAssembly::LOOP_S:
printAnnotation(OS, "label" + utostr(ControlFlowCounter) + ':');
ControlFlowStack.push_back(std::make_pair(ControlFlowCounter++, true));
return;
case WebAssembly::BLOCK:
case WebAssembly::BLOCK_S:
ControlFlowStack.push_back(std::make_pair(ControlFlowCounter++, false));
return;
case WebAssembly::TRY:
case WebAssembly::TRY_S:
ControlFlowStack.push_back(std::make_pair(ControlFlowCounter, false));
TryStack.push_back(ControlFlowCounter++);
EHInstStack.push_back(TRY);
return;

For example,

block (i32) -> () ;; Block input parameter error
end_block         ;; Now this errors out as "End marker mismatch"

This is confusing because there is a block and the end_block is not a mismatch. Only that block has a type checking error, but that's not an end marker mismatch.

I think we can just print the instruction whether we had a type checking error or not, and this will be less confusing.


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

2 Files Affected:

  • (modified) llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp (+2-2)
  • (added) llvm/test/MC/WebAssembly/annotations-typecheck.s (+11)
diff --git a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
index 5ea6c6bc0758b9..ee8686d1166a5b 100644
--- a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
+++ b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
@@ -1157,8 +1157,8 @@ class WebAssemblyAsmParser final : public MCTargetAsmParser {
           Inst.setOpcode(Opc64);
         }
       }
-      if (!SkipTypeCheck && TC.typeCheck(IDLoc, Inst, Operands))
-        return true;
+      if (!SkipTypeCheck)
+        TC.typeCheck(IDLoc, Inst, Operands);
       Out.emitInstruction(Inst, getSTI());
       if (CurrentState == EndFunction) {
         onEndOfFunction(IDLoc);
diff --git a/llvm/test/MC/WebAssembly/annotations-typecheck.s b/llvm/test/MC/WebAssembly/annotations-typecheck.s
new file mode 100644
index 00000000000000..1fcdb9c0e35878
--- /dev/null
+++ b/llvm/test/MC/WebAssembly/annotations-typecheck.s
@@ -0,0 +1,11 @@
+# RUN: not llvm-mc -triple=wasm32-unknown-unknown < %s | FileCheck %s
+
+# This tests annotations are handled correctly even if there is a type checking
+# error (which are unrelated to the block annotations).
+test_annotation:
+  .functype   test_annotation () -> ()
+  block (i32) -> ()
+    drop
+# CHECK-NOT: # End marker mismatch!
+  end_block
+  end_function

@@ -0,0 +1,11 @@
# RUN: not llvm-mc -triple=wasm32-unknown-unknown < %s | FileCheck %s
Copy link
Member

Choose a reason for hiding this comment

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

so this means that the assembly as a whole still fails, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It fails because type checking fails. But at least InstPrinter and its annotation printer is not printing confusing messages.

@aheejin aheejin merged commit 1753d16 into llvm:main Oct 4, 2024
11 checks passed
@aheejin aheejin deleted the inst_printer_tc branch October 4, 2024 00:32
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
When there was a type checking error, we didn't run `InstPrinter`. This
can be confusing because when there is an error in, say, block parameter
type, `InstPrinter` doesn't run even if it has nothing to do with block
parameter types, and all those updates to `ControlFlowStack` or
`TryStack` do not happen:

https://github.com/llvm/llvm-project/blob/c20b90ab8557b38efe8e8e993d41d8c08b798267/llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp#L135-L151

For example,
```wast
block (i32) -> () ;; Block input parameter error
end_block         ;; Now this errors out as "End marker mismatch"
```
This is confusing because there is a `block` and the `end_block` is not
a mismatch. Only that `block` has a type checking error, but that's not
an end marker mismatch.

I think we can just print the instruction whether we had a type checking
error or not, and this will be less confusing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:WebAssembly mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants