search

briansmith / ring

Safe, fast, small crypto using Rust
Other
3.64k stars 682 forks source link

CI: Don't pass `-march` to GCC for s390x. #2078

Closed briansmith closed 4 weeks ago

briansmith commented 1 month ago

Use the defaults.

briansmith commented 1 month ago

@uweigand CI seems to pass with this change. Is there any reason we shouldn't do it?

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.23%. Comparing base (8238c00) to head (8a6973e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2078 +/- ## ========================================== - Coverage 97.24% 97.23% -0.01% ========================================== Files 144 144 Lines 19995 19995 Branches 228 228 ========================================== - Hits 19444 19443 -1 Misses 525 525 - Partials 26 27 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

uweigand commented 1 month ago

@uweigand CI seems to pass with this change. Is there any reason we shouldn't do it?

The CI still seems to be using the stock Jammy qemu (6.2), which still has the bug https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg06965.html. This means if the generated code contains the z13 instruction LOCFHR, qemu will mistranslate it and execution will fail.

Now whether or not the generated code does contain the instruction of course depend both on the source code and the compiler level, so any change to any of that might cause the instruction to appear or disappear. So even if in the current state, the instruction is not used and CI passes, any random change might cause the instruction to appear again, which would make the CI fail again.

So I'd prefer to leave the -march=zEC12 in by default, until such time as the CI server moves to use a recent-enough qemu build that no longer has this bug.

briansmith commented 4 weeks ago

The CI still seems to be using the stock Jammy qemu (6.2), which still has the bug https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg06965.html. This means if the generated code contains the z13 instruction LOCFHR, qemu will mistranslate it and execution will fail.

That is a good point. OTOH, if that starts happening again then it will affect users of ring on s390x that don't override the target CPU. It would be good for us to get early warning about that so we are affected in the same way as a typical user. In general we try to keep our test build environment "stock" as much as possible. So I'm going to merge this and see what happens.