[threads] Shared basic heap types

[tlively]

Implement binary and text parsing and printing of shared basic heap types and
incorporate them into the type hierarchy.

To avoid the massive amount of code duplication that would be necessary if we
were to add separate enum variants for each of the shared basic heap types, use
bit 0 to indicate whether the type is shared and replace getBasic() with
getUnsharedBasic(), which clears that bit. Update all the use sites to record
whether the original type was shared and produce shared or unshared output
without code duplication.

[kripken]

How about this?

enum Shareability { Shared, Unshared }; .. getBasic(Shareability shared = Unshared)

Then existing callsites don't need to change, and also it avoids a function getSharedBasic(..) which might return a non-Shared result (that seems like it could avoid confusion). There could also be helpers (though maybe they are not needed),

getSharedBasic() { return getBasic(Shared); } getUnsharedBasic() { return getBasic(Unshared); }

[tlively]

It's important that getBasic doesn't exist after this change. If it did exist, users might not consider sharedness when they use it. When the only options are getUnsharedBasic and getSharedBasic`, it's much harder to forget to handle sharedness correctly.

Regarding the Shareability enum, I don't think it's necessary because there's a clear correspondence with shared as true and unshared as false. Nobody would think that true mean unshared they way they might confuse e.g. nullability. If you disagree and think the enum is worth it, I can add it.

[kripken]

Fair enough, I agree it's important to force users to consider sharedness. How about getBasic(Shareability shared) then, with no default? I think that's better because of the reason I said before, that it seems slightly confusing for a function getSharedBasic to not always return a shared result.

[tlively]

The "shared" in getSharedBasic(bool) is more like "shared?" in that the parameter says whether it will actually be shared or not. I don't think that's too confusing. I'd prefer getMaybeSharedBasic(bool), but that's way too verbose.

[kripken]

I thought on this some more on the weekend and I still think getSharedBasic(bool) is not ideal. I get that we could get used to getSharedBasic(bool), but for a new reader, or maybe after not using the API for a time, it isn't immediately obvious if the bool refers to the shared or the basic. That is, the name suggests it is getting something both shared and basic - which does the bool refer to?

It doesn't feel like say setOpen(bool = true) where there is just one possibility, so is clear that "open" refers to the bool. That's just a simple setter function, really.

I don't feel strongly between less ambiguous options like getSharedBasic() / getUnsharedBasic() or getBasic(Sharability). I just think it would be best if reading the code makes it really obvious what the API does. But maybe there's a better option?

[tlively]

I'll try switching to getGBasic(Shareability) and see what I think.

[tlively]

See the latest commit. I still prefer it the way it was; the APIs using Shareability are generally more verbose and it seems like an unnecessary complication over using a bool.

[kripken]

I agree this part is less good,

- typeBuilder[i].setShared(type.isShared()); + typeBuilder[i].setShareability(type.getShareability());

But maybe that could be

 typeBuilder[i].setShared(type.getShared()); 

? That is, I think we can call the setter setShared and have a getter getShared; Shareability can be the enum name, but the getters/setters can be more concise.

Other than that the last commit looks like an improvement to me actually. Even some parts that get a bit longer look like they benefit in clarity,

- HeapTypeT makeFuncType(bool) { return Ok{}; } + HeapTypeT makeFuncType(Shareability) { return Ok{}; }

(before, whether the bool was shared or open or some other property is not obvious).

I don't have an opinion on

- type.getUnsharedBasic() + type.getBasic(Unshared)

I think we could still use the old form there.

[kripken]

Another option is to have (instead, or in addition for convenience) boolean-using getters/setters setShared(bool), bool isShared() as those are unambiguous, and use Shareability in ambiguous contexts. We do something similar for signed iirc.

[tlively]

Ok, I updated {get,set}Shareability to {get,set}Shared and for the setters made the default parameter Shared. I'm happy with this.

[kripken]

This internal flag could maybe be a bool just for consistency with the two above it, but if it's more convenient otherwise sgtm as is.

[tlively]

I'd rather avoid mixing bool and Shareability to avoid future questions of whether we should use one or the other in specific cases.

[kripken]

I would expect to see if a.shared != b.shared return {} since I assume the LUB of func and (shared func) is empty. Is that not so?

Instead IIUC this computes the LUB on the unshared variants of them both, then makes that LUB shared if a is. Why is that correct? (and why is it ok to pick the shareability from a specifically?)

[tlively]

That's covered by the HeapType(a).getTop() != HeapType(b).getTop() check up above. Shared and unshared types will always have different top types.

[kripken]

Ah, right, thanks!

[kripken]

If we add overloads setShared(bool) then this could be a little shorter and I think just as unambiguous and clear, but I don't feel strongly.

[tlively]

Same reason to avoid mixing bool and Shareability applies here.

[kripken]

lgtm % question

[kripken]

Was this just wrong before? It does look right now. What fixed it?

[tlively]

Hehe, yes, this was just wrong: https://github.com/WebAssembly/binaryen/pull/6667/files#diff-666d70a4550a2ccec6247c639d54231a05108efc40af830403dfbc6527b251f7R2087-R2088

[kripken]

I missed that on my read, good catch.