cranelift: Fix trampoline args for b1 types

[afonso360]

Hey,

We're now having issues with this on the fuzzer, since we introduced b1 types in #3094.

The assert was complaining because we identify DataValue::B's as B8's. I think we are ok disabling this assert for bools, because we are always going to have a size mismatch, and this check is not meaningful for this type.

Fixing trampoline bool args is tracked in #2237

[bjorn3]

b* are written as rust bool which is 1 byte big. For eg b16 2 bytes would need to be written to ensure that the bool is actually valid. 0xff00 is not a valid bool afaik. In addition this would only work somewhat fine on little-endian systems. On big endian systems, the msb and not the lsb of the bool would be written.

[afonso360]

So, I'm guessing the solution here would be to write these as u128's in write_value_to:

wasmtime/cranelift/codegen/src/data_value.rs

Line 68 in 065190f

DataValue::B(b) => ptr::write(p as *mut bool, *b),

To ensure that we always clear all bytes, but I'm not sure how this would work on big endian systems.
We always reserve a u128 for each slot, so this shouldn't write values out of bounds.

0xff00 is not a valid bool afaik

Aren't larger boolean sizes meant to work as bitmasks? That is the general impression that I got so far, but I haven't seen anything concrete about this.

[bjorn3]

A b16 would be either stored as 0x0000/0xffff or 0x0000/0x0001. I am not sure which one is the right one. 0xff00 is definitively not right.

[afonso360]

I updated this to write out the full u128 slot as 1 or 0, but lets wait on some feedback about writing it as all ones instead

[abrown]

Here's context from the docs:

The b1 type represents an abstract boolean value. It can only exist as an SSA value, and can't be directly stored in memory. It can, however, be converted into an integer with value 0 or 1 by the bint instruction (and converted back with icmp_imm with 0).
Several larger boolean types are also defined, primarily to be used as SIMD element types. They can be stored in memory, and are represented as either all zero bits or all one bits.

I've had some conversations about this with @sunfishcode and @cfallin in the past--I'll let them comment here. FWIW, I think the approach in ecb72cc of storing a 0 or a 1 to represent a boolean is fine.

[cfallin]

Just clearing some backlog and seeing this now -- sorry for the delay! A few points:

• Wider bool types are indeed stored as all-ones or all-zeroes, as they're meant to serve as bitmasks.
• For b1, as @abrown notes above, they're not supposed to be stored/loaded from within CLIF, semantically. On all three of x64, aarch64, s390x, we AND out the LSB to implement bint, following the usual "upper bits are undefined" invariant.
• But because we are writing runtime-internal code, we can reason about the combination of our runtime-side stores and generated-code loads; we aren't restricted by "don't load/store b1".

If we are going to do the fully generic thing for bools of all widths, the proper approach I think is to write all-ones (-1i128 as u128) to the u128 slot, then a load of any width will pick up all-ones. So I think this patch is almost there, except for the constant value.

[cfallin]

Sorry for the delay -- thanks for the updates! LGTM now.