Save local set data in EndTryTable

[zherczeg]

I have checked with gdb that OnEnd is really called. I hope I don't make more mistakes. I have also added the test case without unreachable, since the unreachable should not affect the bug, although it might trigger future bugs, so both should be present.

[zherczeg]

Ok try tables use a very different mechanism compared to normal blocks. I was able to trigger the issue in #2670 again with adding a local ref.

I checked the push / pop labels then.
Validator::BeginTryTableExpr -> SharedValidator::EndTryTable -> TypeChecker::EndTryTable -> TypeChecker::PushLabel
ExprVisitor::VisitExpr -> Validator::EndTryTableExpr -> SharedValidator::OnEnd -> TypeChecker::OnEnd -> TypeChecker::PopLabel

It looks like the BeginTryTable does not construct any labels. Probably because no code is generated then. The label construction happens at EndTryTable. Then the expressions are visited, and OnEnd is called by EndTryTableExpr. I don't know how the interpreter handles this, probably using its OnEndExpr.

I would need some help to understand how this code should work. Maybe more complex test cases are needed.

[zherczeg]

If the code block of a try table is between EndTryTable .. EndTryTableExpr (=OnEnd), and the catch handlers has no side effects in SharedValidator, this patch is correct. Since I am not familiar with the try table implementation, there might be other rules.

[sbc100]

Is this is test that is missing from the upstream repo? Perhaps we should file an issue there, with the hope that can one day remove this downstream test?

[zherczeg]

I am not sure I understand this question. I have created this test myself. The test checks that the "is set" bitset is correctly working. My code was wrong, I saved the bitset in a wrong place. I misunderstood the API sequence order for try_tables.

[zherczeg]

Ah, you might refer to the failure. That is intentional, the WebAssembly set-before-use check is very simple.

The spec test interpreter also yields this error (although stops at the first error):
test.wast:13.17-13.18: validation error: uninitialized local

[sbc100]

My question is basically this: If this is an interesting test case, why can't it be (or shouldn't it be) upstream in the spec repo. I guess this is question we should be asking of all of our local/downstream tests.

[sbc100]

In any case this change LGTM

[zherczeg]

If they are interested, why not. Anyway, it is possible that a similar test is already present in the spec test repo. The GC/function-references proposal don't have such test, since (reworked) exceptions is another proposal, but the full WebAssembly 3.0 test system may have.