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.

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

Validate pops while ignoring nameless blocks? #6950

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Validate pops while ignoring nameless blocks? #6950

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e374e6e
start
kripken Sep 17, 2024
cebcc7e
tests
kripken Sep 17, 2024
330da85
update a lit test
kripken Sep 17, 2024
0298f18
progress
kripken Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions src/wasm/wasm-ir-builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,17 +1441,29 @@ Result<> IRBuilder::makePop(Type type) {
// already create them automatically when starting a legacy catch block that
// needs one. Just verify that the Pop we are being asked to make is the same
// type as the Pop we have already made.
auto& scope = getScope();
if (!scope.getCatch() || scope.exprStack.size() != 1 ||
!scope.exprStack[0]->is<Pop>()) {
return Err{
"pop instructions may only appear at the beginning of catch blocks"};
}
auto expectedType = scope.exprStack[0]->type;
if (!Type::isSubType(expectedType, type)) {
return Err{std::string("Expected pop of type ") + expectedType.toString()};
//
// We need to look up the scope stack, as we can look through nameless blocks.
Index i = 0;
while (1) {
auto scopeCheck = getScope(i++);
CHECK_ERR(scopeCheck);
auto& scope = **scopeCheck;
if (auto* block = scope.getBlock()) {
if (!block->name.is()) {
continue;
}
}
if (!scope.getCatch() || scope.exprStack.size() != 1 ||
!scope.exprStack[0]->is<Pop>()) {
return Err{
"pop instructions may only appear at the beginning of catch blocks"};
}
auto expectedType = scope.exprStack[0]->type;
if (!Type::isSubType(expectedType, type)) {
return Err{std::string("Expected pop of type ") + expectedType.toString()};
}
return Ok{};
Copy link
Member

Choose a reason for hiding this comment

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

This change avoids the error, but it doesn't actually move the pop into the right place. Whenever we try to pop from an empty stack, we should look up the stack through nameless blocks to find pops that we could use.

Weird edge case to test:

  • We lazily give unnamed blocks names when we encounter branches that target them via an index. What happens if we have previously tried to pull a pop through such a block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this a situation of popping from an empty stack? Are Pop instructions not added to the stack like anything else? (they have a concrete type)

Copy link
Member

@tlively tlively Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, pops are special. They are automatically pushed onto the stack when the beginning of a catch is parsed, not when the pop instruction itself is parsed. That's why the existing code here just does some error checking and doesn't push anything. The reason is that we fully support parsing standard code that does not contain any pops, so we can't wait until we see a pop to push it onto the stack. For example, this parses:

try
catch $e
 drop
end

Copy link
Member

Choose a reason for hiding this comment

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

To avoid the edge case here, we should use a scratch local to pull the value of the pop into the block. That won't help the example I gave below, though, so we still need to relax the validation behavior as well.

}
return Ok{};
}

Result<> IRBuilder::makeRefNull(HeapType type) {
Expand Down
29 changes: 29 additions & 0 deletions test/lit/validation/eh-nameless.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
;; Test for a nameless block containing a pop, which is allowed.

;; XXXXXXXXXXXXXXX RUN: not wasm-opt --enable-reference-types %s -o - -S 2>&1 | filecheck %s --check-prefix NO-GC
;; RUN: wasm-opt --enable-reference-types --enable-gc %s -o - -S | filecheck %s --check-prefix GC

;; NO-GC: all used types should be allowed

;; GC: (func $foo (type $0) (param $x eqref)

(module
(tag $tag (param i32))

(func $0
(try
(do
(nop)
)
(catch $tag
(drop
(block (result i32)
(pop i32)
)
)
)
Comment on lines +18 to +24
Copy link
Member

Choose a reason for hiding this comment

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

This Wasm doesn't really make sense from a typing perspective unless the block takes an i32 parameter, which we don't yet support. I'm not sure we should want to be able to parse something like this.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we probably should not be printing text that looks like this.

I'm working on using IRBuilder in the binary parser now, after which we will be able to update it to support control flow parameters, so we could fix this example to have proper typing, but even then, IRBuilder will lower the control flow parameters away using locals, so we still wouldn't be able to parse this IR directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I see what you mean. But this is consistent with our other extensions to the text format, e.g., this validates:

(module
  (func $foo
    (local $x (ref func))
    (block
      (local.set $x
        (ref.func $foo)
      )
    )
    (drop
      (local.get $x)
    )
  )
)

That isn't valid wat since the block should end the range of the non-nullable local, but we decided to allow it.

With that said, I do see that this change would be more intrusive and confusion, potentially. So maybe this PR is a bad idea.

But we do have a general problem here, where more passes then we expected need EH fixups. Modifying them all adds clutter and overhead, so I was hoping for a more general solution...

Copy link
Member

Choose a reason for hiding this comment

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

This Wasm doesn't really make sense from a typing perspective unless the block takes an i32 parameter, which we don't yet support. I'm not sure we should want to be able to parse something like this.

I don't think I understand. Why does this not make sense unless the block takes an i32 input parameter? What does this have to do with an input parameter?

Copy link
Member

Choose a reason for hiding this comment

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

pop i32 models popping a value off the stack and then pushing it right back onto the stack to be consumed by its parent expression, so it's similar to a nop, except that it requires a value to be on the stack in the first place. In this example, there would only be a value available on the stack if the block took an i32 parameter.

In the long run, I hope that the text and binary parsers will be able to lower legacy EH directly to new EH IR, so all the problems with pop validation will become moot. Maybe there are less intrusive changes we can make in the short term?

)
)
)

;; TODO: test two nested blocks
Loading