Fix s390x regressions by uweigand · Pull Request #3330 · bytecodealliance/wasmtime

uweigand · 2021-09-11T13:15:54Z

Add relocation handling needed after PR Use relative call instructions between wasm functions #3275
Fix incorrect handling of signed constants detected by PR aarch64: Fix incorrect encoding of large const values in icmp. #3056 test
Disable fuzzing tests that require pre-built v8 binaries
Disable cranelift test that depends on i128
Temporarily disable memory64 tests

uweigand · 2021-09-11T13:17:09Z

Note: In addition to this PR, the endian fix in PR #3329 and the rsix build fix bytecodealliance/rustix#53 are required to make the CI tests fully pass again on s390x.

uweigand · 2021-09-11T13:21:32Z

Also: Using qemu 6.1 we should now be able to turn on upstream CI for the s390x platform.

github-actions · 2021-09-11T13:33:06Z

Subscribe to Label Action

cc @fitzgen

Details

This issue or pull request has been labeled: "cranelift", "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

This pulls in the s390x fix needed by bytecodealliance#3330. Also a small `rsix` API update; `PollFdVec` has been removed in favor of just using `Vec<PollFd>`.

sunfishcode · 2021-09-11T16:00:55Z

I opened #3331 to pull in the s390x fix in rsix.

uweigand · 2021-09-11T16:54:31Z

I opened #3331 to pull in the s390x fix in rsix.

Thanks, @sunfishcode !

This pulls in the s390x fix needed by #3330. Also a small `rsix` API update; `PollFdVec` has been removed in favor of just using `Vec<PollFd>`.

alexcrichton

I think someone else will want to review the cranelift/codegen/src/isa/s390x/lower.rs bits, but all the other bits look good to me.

alexcrichton · 2021-09-13T13:58:26Z

 // No simd support yet for s390x.
 ("simd", _) if platform_is_s390x() => return true,
+ // No memory64 support yet for s390x.
+ ("memory64", _) if platform_is_s390x() => return true,


How come memory64 isn't implemented on s390x? Does it lower to unimplemented clif things at the moment?

Yes. This is all fixable, but I'd prefer to do that separately.

alexcrichton · 2021-09-13T13:58:59Z

 // This can address any valid CodeOffset.
 LabelUse::BranchRIL => 0x7fff_ffff,
 LabelUse::PCRel32 => 0x7fff_ffff,
+ LabelUse::PCRel32Dbl => 0x7fff_ffff,


Given the shift, is the range actually one bit larger than what's specified here?

The range is actually +/- 4GB, not +/-2 GB. But that apparently cannot be represented as CodeOffset anyway, so I simply accept all valid CodeOffsets here.

Could this return 0xffff_ffff as a placeholder? One day I think it'd be good to represent some code offsets in this area as a 64-bit integer, but that I think is a much bigger change.

I tried that when adding BranchRIL (which has the same range) initially, and it caused failures (some kind of overflows in common code), so that's what I ended up with. I guess this could be revisited at some point, but then we should do it for all affected LabelUse types, not just PCRel32Dbl.

Can the overflow be fixed with a saturating_add or something like that somewhere? If not can the comment be updated that this should be 4GB but it's specified as 2GB for now?

That would probably help, I'll give it a try. It's still better to do that in a separate PR I think.

- Add relocation handling needed after PR bytecodealliance#3275 - Fix incorrect handling of signed constants detected by PR bytecodealliance#3056 test - Fix LabelUse max pos/neg ranges; fix overflow in buffers.rs - Disable fuzzing tests that require pre-built v8 binaries - Disable cranelift test that depends on i128 - Temporarily disable memory64 tests

uweigand · 2021-09-13T15:42:06Z

@alexcrichton you were right, I just had to add a single saturating_add. Given that this is just a one-line obvious fix to common code, I've now updated this PR after all. Let me know if you'd prefer me to take it out again.

Note that BranchRI max ranges were just completely wrong, but that type isn't currently used anywhere so it didn't matter. I've now fixed this and used precise max ranges for all types, only capped by a max of 0xffff_ffff due to the u32 type.

alexcrichton · 2021-09-13T20:07:58Z

Nah yeah that's what I would have expected for a saturating_add and it looks good to me! I think someone will still want to look over the sconst changes to the s390x backend though.

afonso360 · 2021-09-15T14:52:31Z

Also: Using qemu 6.1 we should now be able to turn on upstream CI for the s390x platform.

Just want to mention that we have now upgraded to 6.1 in #3325

uweigand · 2021-09-15T15:27:58Z

Nah yeah that's what I would have expected for a saturating_add and it looks good to me! I think someone will still want to look over the sconst changes to the s390x backend though.

Thanks! @cfallin do you want to have a look at those?

uweigand · 2021-09-15T15:29:00Z

Also: Using qemu 6.1 we should now be able to turn on upstream CI for the s390x platform.
Just want to mention that we have now upgraded to 6.1 in #3325

Excellent. Not sure exactly what we need to do to add a CI job for s390x, I haven't done much with GitHub actions before ...

alexcrichton · 2021-09-15T15:45:46Z

To add a CI job theoretically all that needs to be done is to copy/paste this block to add another entry below it, and then update the various values for s390x instead of aarch64.

uweigand · 2021-09-15T15:49:52Z

To add a CI job theoretically all that needs to be done is to copy/paste this block to add another entry below it, and then update the various values for s390x instead of aarch64.

Thanks! I'll give it a try once the two PRs are merged.

cfallin

Thanks! @cfallin do you want to have a look at those?

The label/MachBuffer changes look good to me!

uweigand · 2021-09-20T10:59:34Z

So can this be merged now? This is now the final bit blocking successful builds on s390x ...

uweigand · 2021-09-20T16:09:32Z

To add a CI job theoretically all that needs to be done is to copy/paste this block to add another entry below it, and then update the various values for s390x instead of aarch64.

This is now #3372 . I also had to disable the lightbeam tests (just like on aarch64).

uweigand mentioned this pull request Sep 11, 2021
RFC: Wasmtime 1.0 bytecodealliance/rfcs#14
Merged

github-actionsBot added cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure labels Sep 11, 2021

sunfishcode mentioned this pull request Sep 11, 2021
Update to cap-std 0.19.0 and rsix 0.22.4. #3331
Merged

alexcrichton pushed a commit that referenced this pull request Sep 11, 2021
Update to cap-std 0.19.0 and rsix 0.22.4. (#3331)
4d86f0c
This pulls in the s390x fix needed by #3330. Also a small `rsix` API update; `PollFdVec` has been removed in favor of just using `Vec<PollFd>`.

alexcrichton reviewed Sep 13, 2021
View reviewed changes

uweigand force-pushed the s390x-regressions branch from 6d3a06e to 1632d0eCompare September 13, 2021 15:39

github-actionsBot added the cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. label Sep 13, 2021

cfallin approved these changes Sep 15, 2021
View reviewed changes

alexcrichton merged commit 51131a3 into bytecodealliance:mainSep 20, 2021

uweigand deleted the s390x-regressions branch September 20, 2021 14:25

Uh oh!

Conversation

uweigand commented Sep 11, 2021

Uh oh!

uweigand commented Sep 11, 2021

Uh oh!

uweigand commented Sep 11, 2021

Uh oh!

github-actionsBot commented Sep 11, 2021

Subscribe to Label Action

Uh oh!

sunfishcode commented Sep 11, 2021

Uh oh!

uweigand commented Sep 11, 2021

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uweigand commented Sep 13, 2021

Uh oh!

alexcrichton commented Sep 13, 2021

Uh oh!

afonso360 commented Sep 15, 2021

Uh oh!

uweigand commented Sep 15, 2021

Uh oh!

uweigand commented Sep 15, 2021

Uh oh!

alexcrichton commented Sep 15, 2021

Uh oh!

uweigand commented Sep 15, 2021

Uh oh!

cfallin left a comment

Choose a reason for hiding this comment

Uh oh!

uweigand commented Sep 20, 2021

Uh oh!

uweigand commented Sep 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants