-
Notifications
You must be signed in to change notification settings - Fork 72
Add support for big-endian platforms. #198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the report and this fix!
But is it complete? Do we need to do anything for getWord
and/or other runtimes?
Also, are you able to pass all tests in this repo and Wasm tests in Envoy with this fix? Sorry, normally I'd verify this myself, but I don't have access to any big-endian machine.
src/v8/v8.cc
Outdated
@@ -441,7 +441,11 @@ bool V8::setWord(uint64_t pointer, Word word) { | |||
if (pointer + size > memory_->data_size()) { | |||
return false; | |||
} | |||
#if BYTE_ORDER == LITTLE_ENDIAN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are those defines coming from? I'm not sure if this is the most portable way to detect endianess. I'd prefer using __BYTE_ORDER__
and friends, since that's supported by both GCC and Clang:
#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
uint32_t word32 = __builtin_bswap32(word.u32());
#else
uint32_t word32 = word.u32();
#endif
Note the different order to fallback to little-endian in case of missing defines.
Thanks for the review, I've changed the code to use
Bazel 'rules_rust' is not supported on s390x: https://github.com/bazelbuild/rules_rust/releases |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll wait with merging it for a bit to see if you can figure out how to run Rust tests.
Could run the tests on s390x with the following local code changes:
So I had to use this fork https://github.com/maistra/proxy-wasm-cpp-host to compile the tests (boringssl is replaced with openssl).
Replaced rust-lld with lld (added tests on s390x:
|
Hi @knm3000, thanks for getting those tests to work! However, you response reminded me that we don't actually run tests against V8 in Proxy-Wasm C++ Host repo (we always relied on testing all the way in Envoy), which obviously isn't great. It took me way too much time, but I finally managed to get V8 working in #204, and if you checkout/cherry-pick that PR, then you should be able to run tests against V8 using: bazel test --runtime=v8 //test/... Note: Bazel support in V8 is still under active development, and I've extended list of source files to include those needed by s390x and added some defines, but it was never tested on a s390x system, so I'm not sure if it works. |
Hi @PiotrSikora, I have built the tests with v8 on s390x using your latest changes, the following extra changes are required for the build: #214 |
However,
|
On the real s390x hardware the test results are the same. without patch: Uncaught RuntimeError: memory access out of bounds
with patch: Uncaught RuntimeError: unreachable
Anyway, it is not good, and I need to fix this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unapproving until this can pass the tests.
Hi @PiotrSikora , I've updated the PR, now the tests are passed on s390x, can you please review? Thanks.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
I'm looking at those changes, and they all (sans the one case with getMemory
) do the same swap in various (but not all) calls to setWord
. I imagine that the remaining instances were omitted because of a poor test coverage in this repository, and not because they aren't needed.
Do you think we could do the swap in Word
constructor in include/proxy-wasm/word.h
? That should solve this at the core.
I tried swapping in This time I put some more calls to __builtin_bswap32 into exports.cc (I found more errors when running a different example https://github.com/proxy-wasm/proxy-wasm-rust-sdk/blob/master/examples/http_headers.rs, and when compiling http_headers.wasm binary for s390x target, I also had to change Anyway, we need to swap only in exports.cc (and not in any other places). The tests still work with the latest update to PR code:
|
Thanks for iterating on this, @knm3000!
I've looked at those changes, and you've wrapped all calls to Once you do that, I suspect that some tests will start failing, since you're swapping only when setting memory, but not when getting it, and a similar change should be done in Also, since exactly the same swap is now done in multiple places, could you add a new function and use it instead of adding
(untested, but something like that should work)
This is wrong. Wasm is little-endian, so everything in its memory space should be little-endian. Instead of the changes in
One reason that those tests are passing, is that you've patched callsites to Also, if you change |
Actually, ignore that. You can simply use |
Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Hi @PiotrSikora, please review the latest commit:
|
Build on macos failed, it looks like htole32 (and other similar functions) are not defined on macos. Should I define them like this |
macOS is always little-endian, so this should be even easier:
Maybe add it to |
That doc might be out-of-date, because they test on Linux/s390x on their CI, and if you make the same changes to It might be a good idea to proactively make them in |
Use htole32() and le32toh() instead of __builtin_bswap32() Moved byte reversals into `setWord()` Defined htole32() and le32toh() for macos Reverse in setWord() for wasmtime, wamr and wavm Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Made the same changes to wasmtime, wamr and wavm. When running with wasmtime 1 test (Clock from exports_test.cc) fails:
It fails during the call to |
Thanks!
That's interesting. This was failing for me prior to your changes, but it works fine now under Docker/QEMU:
Granted, it's QEMU and not the real hardware, but still... Anyway, I can play around with it in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating on this, and figuring out how to build it in the first place. Great work!
Note: I believe that there are still some hostcalls where we don't do the correct byteswap, but we don't have a good test coverage, so I won't hold back this PR because of it.
Fixes https://issues.redhat.com/browse/MAISTRA-2648 The permanent fix is not a part of maistra/envoy yet: proxy-wasm/proxy-wasm-cpp-host#198
Fixes https://issues.redhat.com/browse/MAISTRA-2648 The permanent fix is not a part of maistra/envoy yet: proxy-wasm/proxy-wasm-cpp-host#198 Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Fixes https://issues.redhat.com/browse/MAISTRA-2648 The permanent fix is not a part of maistra/envoy yet: proxy-wasm/proxy-wasm-cpp-host#198 Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Fixes https://issues.redhat.com/browse/MAISTRA-2648 The permanent fix is not a part of maistra/envoy yet: proxy-wasm/proxy-wasm-cpp-host#198 Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Fixes https://issues.redhat.com/browse/MAISTRA-2648 The permanent fix is not a part of maistra/envoy yet: proxy-wasm/proxy-wasm-cpp-host#198 Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Fixes https://issues.redhat.com/browse/MAISTRA-2648 The permanent fix is not a part of maistra/envoy yet: proxy-wasm/proxy-wasm-cpp-host#198 Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Fixes WASM on s390x https://issues.redhat.com/browse/MAISTRA-2648 The permanent fixes are: proxy-wasm/proxy-wasm-cpp-host#198 proxy-wasm/proxy-wasm-cpp-host#282 Signed-off-by: Konstantin Maksimov konstantin.maksimov@ibm.com
Fixes WASM on s390x https://issues.redhat.com/browse/MAISTRA-2648 The permanent fixes are: proxy-wasm/proxy-wasm-cpp-host#198 proxy-wasm/proxy-wasm-cpp-host#282
Fixes WASM on s390x https://issues.redhat.com/browse/MAISTRA-2648 Corresponding proxy-wasm/proxy-wasm-cpp-host PRs: proxy-wasm/proxy-wasm-cpp-host#198 proxy-wasm/proxy-wasm-cpp-host#282
Fixes WASM on s390x https://issues.redhat.com/browse/MAISTRA-2648 Corresponding proxy-wasm/proxy-wasm-cpp-host PRs: proxy-wasm/proxy-wasm-cpp-host#198 proxy-wasm/proxy-wasm-cpp-host#282
Fixes WASM on s390x https://issues.redhat.com/browse/MAISTRA-2648 Corresponding proxy-wasm/proxy-wasm-cpp-host PRs: proxy-wasm#198 proxy-wasm#282
Fixes https://issues.redhat.com/browse/MAISTRA-2648 The permanent fix is not a part of maistra/envoy yet: proxy-wasm/proxy-wasm-cpp-host#198 Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Fixes https://issues.redhat.com/browse/MAISTRA-2648 The permanent fix is not a part of maistra/envoy yet: proxy-wasm/proxy-wasm-cpp-host#198 Signed-off-by: Konstantin Maksimov <konstantin.maksimov@ibm.com>
Wasm binary always has Little-endian byte ordering. V8 is hardcoded to reverse the bytes with every wasm load/store operation done on Big-endian machines. But in case of envoy proxy the wasm binary file is linked with envoy C code compiled on a native BE machine. In this case a BE value is being written to LE enforced memory, and the crash happens when running wasm on BE machine. To fix this, everything needs to be written to memory in LE order when linking with any other libraries outside the wasm binary itself.
Resolves #197