LTS: Fix build failure on s390x/musl#13489
Conversation
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks for the PR. To confirm, have you run this code and tested it? I ask because the fp in the glibc case does a dereference of the 15th register, but the code here does not do a dereference. That looks like a bug to me, but I wanted to ask. If you haven't tested this I'm personally a bit hesitant to land this because it's generally much more difficult to diagnose these sorts of issues after it lands.
Additionally, do you know the status of exposing the context structures in musl?
And, finally, can you send this PR to the main branch first, and then cherry-pick it for a backport once it's accepted on main? We don't have a ton of precedent here but I think it'd be best to land things on main first and then backport from there.
github-actions
Bot
added
the
wasmtime:api
|
We built the downstream Alpine packages with this patch, and https://github.com/open-policy-agent/opa (a dependency of wasmtime) built fine with it and passes all tests. I was also going to say that all tests passed with this patch, but noticed that tests were actually disabled for the wasmtime package. I enabled them and some tests actually fail even with this patch, so I'll inspect closer and report back. |
| // Register 1 on s390x is the PSW address | ||
| pc: (cx.gregs[1] - trap_offset) as usize, | ||
| // Register 15 is the frame pointer on s390x | ||
| fp: cx.gregs[15] as usize, |
There was a problem hiding this comment.
Thanks for running tests! I think this may be at least one minor issue (it's different from the fp definition above which reads memory). I'm not sure if S390xRegs is the correct definition, as well.
Also, and I'm sorry for lots of comments, but mind renaming S390xRegs to ucontext_t? That matches the intention of the structure where it's matching the C definition of ucontext_t
2 participants
Patch-Source: https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/83794