Rot13 test fails on s390x

[alexreinking]

When running the unittests on an s390x Docker image, the Rot13 test fails.

[ RUN      ] InterpTest.Rot13
../src/test-interp.cc:546: Failure
Expected equality of these values:
  "Uryyb, JroNffrzoyl!"
  string_data
    Which is: "Hello, WebAssembly!"
[  FAILED  ] InterpTest.Rot13 (8 ms)

Here is a link to a GitHub Actions workflow that shows this failure: https://github.com/alexreinking/wabt/runs/8052934337?check_suite_focus=true

[SoniEx2]

yeah, we're not sure what the best approach to fix this test is. the thing is wabt exposes the wasm memory directly, but on s390x we use a trick to implement LE semantics on a BE platform which just swaps the memory around. another alternative would be doing proper byte swaps for reads and writes, but some platforms (particularly the ones we wanted to use this on) don't support them (or at least, not efficiently) which makes it really slow.

we could either encapsulate the memory (and change the API for big endian platforms, tho it's not like there's a standard wasm C API for big endian hosts anyway) or leave it up to the user (in this case, the test). personally we'd lean towards the latter.

[keithw]

My guess is that either we include s390x in the testsuite (in which case, the latter approach you described sounds fine to me -- but I'm not sure if that's the only failing test or if we're going to have trouble with wasm2c, etc.), or we don't, in which case big-endian support will probably bitrot.

Do we think that WABT has actual users (or likely future users) on big-endian platforms? Given the limited resources, it's probably not worth spending a lot of effort on getting and keeping this running on big-endian unless we think it is helping somebody...

[alexreinking]

Just so there's no confusion from anyone unaware of the context under which I opened this issue...

Do we think that WABT has actual users (or likely future users) on big-endian platforms?

I do not use wabt on any big endian system. I was just trying to improve the build system's platform detection when I hit this bug. I would have been just as happy to help remove big endian support.

[SoniEx2]

the C/C++ API tests, particularly the ones that deal with memory (which happens to be just the rot13 test), are the only failing tests on s390x/big-endian.

(wabt doesn't appear to test the full extent of WASI at this point, but WASI would also be relevant for this. we lean towards requiring the WASI implementation to deal with BE environments, with awareness of the wasm runtime, since there's no standard wasm C/C++ API for dealing with BE environments and there are 2 competing ways of implementing wasm on a BE environment.)

we do want to use wabt for a BE project, but we've been backlogged with our other projects.

[keithw]

Checking in -- if there are people who care about using WABT on big-endian architectures (or s390x specifically), I think we'd love to merge a fix for this, which would then let us merge the s390x parts of #1971 (CI for s390x) and lock this in going forward.

Otherwise I think we should close #1971 / #1972 / #2070 as wontfix (for now) and declare big-endian as unsupported until we have a contigent of people who care.

[SoniEx2]

@keithw has there been a decision about how to expose wasm memory on BE platforms? wabt currently exposes it backwards. if it were to be exposed forwards, that would require byteswaps when reading from wasm memory, while the current approach requires only simple address calculations and is very amenable to certain optimizations (like how a compiler might optimize loops). either way, on BE platforms, it is required to copy from wasm memory to native memory in the general case.

[keithw]

@keithw has there been a decision about how to expose wasm memory on BE platforms?

No particular knowledge/views myself. Ideally we'd do something that:

• is the same as what V8 and Wasmtime do on these platforms (and if the wasm-c-api specifies a particular behavior, matches that too),
• meets the expectations/desires of actual users on big-endian systems, and
• isn't a big lift from where we are now (because I'm doubtful big-endian support is worth a lot of disruption).

[SoniEx2]

we implemented BE support in wabt with old macs (M680x0) and the like in mind, particularly for use with wasm2c, so the performance tradeoffs for modern platforms may be different. our goal isn't just "it works", we want it to work well too - ideally without too much of a slowdown or code size overhead.

it appears wasmtime exposes it as a raw little-endian chunk of memory, which is incompatible with what wabt is currently doing. personally we'd rather try getting wasmtime in alignment with wabt rather than the other way around, due to the previously mentioned performance benefits (particularly on old platforms). but for the time being, there are 2 competing ways of doing it, and they each have their own benefits and drawbacks.

[SoniEx2]

we tried to implement little-endian host view on big-endian hosts and we feel like it isn't gonna work. we were gonna reimplement the memory operations to always follow little-endian order regardless of host and compiler settings, by reading/writing individual bytes and letting the compiler optimize it as possible. this would've simplified things by having a single code path for both BE and LE, however...

then there's atomics.

"leonbe" semantics (the technique currently implemented in wabt) enable us to actually support atomics using host intrinsics, despite wasm being LE while the host is BE. further, this isn't something that can be emulated through reading/writing individual bytes, tho it could still be emulated through the use of compare-and-swap - at a massive cost to performance. unfortunately the simplicity gained from the shared code path would be completely lost by the corresponding atomics support code.

rather than remove BE host support altogether, and acknowledging the challenges around it, we suggest wabt should simply not support the wasm-c-api on BE platforms at this point, instead requiring embedders to use wabt's own APIs. while this wouldn't actually solve the problem (there's nothing really requiring anyone to implement BE host support in their host/native modules), it would avoid clashing with the rest of the wasm ecosystem; as wabt already does not (properly) implement wasm-c-api on BE platforms, wabt already does not (properly) support the wasm-c-api on BE platforms, and wabt already is incompatible with wasm-c-api consumers, the easiest thing to do is to be explicit about it. this would also mean not running/skipping the wasm-c-api tests on BE hosts.

it would be easy enough to revisit this later if, for example, WebAssembly/design#1478 went through. but, at least for the time being, we do believe this would be the best way to avoid confusion.