Should non-nullable locals be initialized by unreachable code? about function-references

[conrad-watt]

It seems totally fine for us to relax the spec to make this code valid (also assuming it's not annoying for implementers), although I imagine that having Binaryen stop emitting code that's program-order-after a statically-known unreachable point (unreachable and unconditional branches) might also be generally valuable for code-size reasons.

[rossberg]

I created #99, which would relax the semantics as suggested. It's not hard but a moderate complication, so we should be perhaps be discussing whether it's worth it.

[conrad-watt]

I don't think I'm clearly understanding whether the code in your linked issue is the input to binaryen or the output. If it's the input, it's invalid, so I'm surprised that Binaryen is attempting to do any kind of optimisation on it at all instead of rejecting it. If it's output, I don't think that there's a clear relaxation of our 1a-style locals that can make it valid.

Put another way, is the issue that Binaryen is emitting code like this:

(block
    unreachable
    local.get $i
end)

(where $i is an uninitialised local)

or code like this?

(block
    unreachable
end)
local.get $i

It's definitely possible to relax validation to make the first code snippet above validate (and in fact we'd need to make this relaxation anyway if we ever wanted to move to the 1b/c cross-block approach) - although I don't understand how code in this form is getting emitted since I'd expect Binaryen to optimise the local.get away.

We can't make the latter code snippet valid without going beyond our current 1a intra-block initialisation tracking.

[tlively]

The code in the linked issue is input code. It doesn't cause any errors because binaryen ignores unnamed blocks when doing non-nullable local validation because it never emits those blocks in its output anyway. So I believe Binaryen is producing code like your first example.

(Note that there also appears to be an opportunity to skip even more unreachable code in Binaryen's binary emitter, which I suppose would also solve this problem. I don't know how complicated that solution would be, though, and regardless it seems useful to relax the spec as much as we can.)

[kripken]

True, yeah. We do that in the --dce pass, so this issue only affects unoptimized code (where code size doesn't matter as much).

Overall if this is a simple and otherwise beneficial relaxation in the spec that could be nice, but if not, we can figure things out in Binaryen of course (but it will add a small amount of work, slowing down builds slightly).

[conrad-watt]

Ah, I hadn't properly understood that some Binaryen configurations might not want to do (syntax-driven) dce. I'm happy to help facilitate relaxing the spec as above (assuming we get a neutral/positive noise from @rossberg and engines).

[titzer]

Would it be possible that Binaryen track the point in a block after which code is unreachable and then avoid adding new instructions to that region, or is the issue that an unreachable gets introduced in the middle of a block?

[tlively]

Yes, Binaryen already does that to some extent, and if it did a better job of it, this issue would go away.

Now that we've identified that fix, I wouldn't say a spec change is strongly motivated by this one problem we ran into. I still think it would be a nice change to make based on the general principle that it is helpful to relax the validation rules to reduce the amount of effort producers have to put into generating valid code.

[titzer]

FWIW I don't think this is a particularly hard change in Wizard's validator; there is literally only one instruction affected, which is local.get, and the control stack entries are annotated with an inUnreachableCode flag.

[rossberg]

From first glance, this looks like a plausible change. But I probably won't have time to look into it for a week or two. It means a few more conditions in every validators, though, so I wonder if it isn't simpler, globally speaking, to implement the fix in Binaryen, which seems desirable anyway.

[tlively]

FWIW this is turning out to be nontrivial to fix in Binaryen (although still desirable to fix, of course). I am unsurprisingly in favor of this spec change.

[rossberg]

I just noticed that this issue is still open. @tlively, is this still relevant for Binaryen?

[tlively]

Yes, I think this is still a problem in principle. We've worked around it by always running DCE in our fuzzer when GC is enabled, though, so it's not a problem that is giving us ongoing trouble. I would be ok closing this issue.