Validate pops while ignoring nameless blocks?

[kripken]

@aheejin The fuzzer found another issue on pops in blocks (see test/lit/passes/gto-removals.wast in this PR), so I started to think that we might want a more general solution here. This draft PR changes validation so that nameless blocks, which vanish in the binary format, are ignored for validation. So makeSequence() etc. will never break pop validation.

This seems to work on the test suite. Do you remember if we considered this before? Is there a problem I am missing?

[aheejin]

I think this makes sense. I don't think we've considered something like this. (Also we need to fix the new parser to handle this too, because currently it rejects parsing if a pop is nested) We omit all unnamed blocks, even if they are typed, right?

But I wonder why was this a problem before, for example, in #4237..? The example in #4237 also an unnamed block, but apparently it caused a validation error in the engine.

[kripken]

(Also we need to fix the new parser to handle this too, because currently it rejects parsing if a pop is nested)

Good catch, the fuzzer found that bug overnight 😄

About the testcase in #4237, looks like it's an unnamed block so we should accept it as valid (rather than fix it up), if we go with this approach. But I'm not sure if I understood your question?

[aheejin]

#4237 seems to say the binary was rejected at compile time from the engine. Does that mean we didn't omit the unnamed blocks in binary then? If we did why would the engine have rejected it?

[kripken]

I see now, sorry for misunderstanding before. Looks like that #4237 is from 2021, and we only added the guarantee of removing such blocks in 2022: the work was in #4943 and related PRs, and we documented the dependency on it when non-nullable local validation landed in #4959

[aheejin]

I see, thanks! I think there are still cases we need fixups but this will remove maybe more than 90% or more of them.

[kripken]

@tlively I could use some advice on how to fix the parser here. This PR requires it to accept code like this:

https://github.com/WebAssembly/binaryen/pull/6950/files#diff-9d8e4085cec9051d98d5f9c2d98eb679d3f5ada3a6fb8800725023595524cb1cR10

In the last commit I believe I fixed up makePop to handle nested scopes like that, but it still errors on parsing, error: popping from empty stack. I added some debug logging (based on IR_BUILDER_DEBUG) and I see this, at the end of the block scope (IRBuilder::finishScope):

Scope stack in function $0:
  scope func 0:
  scope catch:
    pop i32
  scope block:

IIUC, the pop is in the catch, not in the block. I suspect that is because of

binaryen/src/wasm/wasm-ir-builder.cpp

Lines 868 to 872 in b381a8c

	// Push a pop for the exception payload.
	auto params = wasm.getTag(tag)->sig.params;
	if (params != Type::none) {
	push(builder.makePop(params));
	}

Does that code need to be changed to put the pop in the right place? I'm not sure how though.

[tlively]

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?

[kripken]

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)

[tlively]

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

[tlively]

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.

[tlively]

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.

[tlively]

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.

[kripken]

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...

[aheejin]

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?

[tlively]

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?

[kripken]

Perhaps another option instead of this PR is, since this is in old EH, we can ignore the "pop must be in a catch" parser error in the fuzzer..?

[tlively]

Hmm, I just ran into a situation where IRBuilder already generates a pop in a block.

(module
 (tag (param i32))
 (func
  try
  catch 0
   i32.const 3
   drop
   drop
  end
 )
)

Generates this IR:

(try
 (do
  (nop)
 )
 (catch $tag$0
  (drop
   (block (result i32)
    (local.set 0)
     (pop i32)
    )
    (drop
     (i32.const 3)
    )
    (local.get $0)
   )
  )
 )
)

I found this due to test/lit/binary/stack-eh-legacy.test when updating the binary parser to use IRBuilder.

The easiest path forward is probably to figure out how to land this PR without causing more problems. The two solutions I see are to do the thing where we reach up the stack and move the pop when parsing a pop in a block or to print nameless blocks as comments so text round tripping newly valid IR does not require the parser to parse a pop in a block.

[kripken]

@tlively Oh, I was thinking we didn't want to land this? Are you saying it would help the IRBuilder work?

For that code sample, running the EH fixups helper should fix things?

[tlively]

@tlively Oh, I was thinking we didn't want to land this? Are you saying it would help the IRBuilder work?

It would fix validation errors that you can get out of IRBuilder today, yes.

For that code sample, running the EH fixups helper should fix things?

Yes, but we probably don't want to do that every time we parse a module, right?

[kripken]

Yes, but we probably don't want to do that every time we parse a module, right?

Yeah, it's the same issue as with passes. We prefer not to run the EH fixups, so we try to run them only when we detect they might be needed, specifically when we add a block around a pop. Some passes have good ways to detect that. If IRBuilder can do so then that might be simpler than this PR, which changes what we accept as valid input.

[tlively]

Ok, I can take a look at that.