[Parser] Fix bug in unreachable fallback logic

[tlively]

When popping past an unreachable instruction would lead to popping from an empty
stack or popping an incorrect type, we need to avoid popping and produce new
Unreachable instructions instead to ensure we parse valid IR. The logic for
this was flawed and made the synthetic Unreachable come before the popped
unreachable child, which was not correct in the case that that popped
unreachable was a branch or other non-trapping instruction. Fix and simplify the
logic and re-enable the spec test that uncovered the bug.

[kripken]

Suggested change

	// not reaching past an unreachable, the error will be caught we
	// when pop.
	// not reaching past an unreachable, the error will be caught when
	// we pop.

[kripken]

This seems to change the meaning of needUnreachableFallback slightly - worth updating the comment on line 379 perhaps?

[tlively]

That comment is actually already correct because it mentions "or try to pop from an empty stack." This fixes the code to agree with the comment.

[kripken]

I am having a hard time following this. "The next item is the unreachable we must not pop past" fits scope.exprStack.size() == *unreachableIndex + 1. Then "this next item be child 0" seems to refer to the same "next item", which suggests *unreachableIndex == 0. Is that right? If so then I can't see why.

[tlively]

No, *unreachableIndex is an index in the stack, not in the children we are popping. i is the index of the child we want to pop, so that's why we check i > 0 here.

[kripken]

That helps, thanks. But if i is the index of the child we want to pop, and we check i > 0 before getting here, how can so this next item must be child 0 be true?

[tlively]

Because inside this condition we are not popping a value from the stack, we are synthesizing an unreachable instead. We're deferring popping that value until i == 0.

[kripken]

I see. Ok, I think I understand the surrounding code a bit better now. But I am still quite confused 😄 Specifically, about this sentence:

We cannot insert unreachables in front of it because it might be a branch we actually have to execute, so this next item must be child 0.

I can't follow the inference of "so". I can try to paraphrase the key part of it as "We can't insert unreachables, as it might be a branch, and therefore it must be child 0". Neither "we can't insert unreachables" nor "it might be a branch" suggest to me any reason why the child must be 0 and not any other index.

[tlively]

If we put the unreachable instruction from the stack in the child 1 slot, then there would be nothing valid to do for the child 0 slot. Popping off the stack would be popping past the unreachable, which we determined we cannot do. Synthesizing an Unreachable for child 0 would insert that Unreachable before the unreachable instruction from the stack in the execution order and change the program semantics if the instruction from the stack was a branch.

[kripken]

Why is that not a problem for the child 2 slot as well? If we put the unreachable instruction from the stack in the child 2 slot then there would be nothing to do for the child 1 or child 0 slots, I think, for the exact reasons you said?

[tlively]

Exactly, it's a problem for any slot except for child 0. That's why the child 0 slot is special.

[kripken]

I see now, thanks... sorry for being dense!

[tlively]

Haha no worries! It's certainly a dense piece of code.