Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CI: Enable overflow checks for test (non-dist) builds #89776

Merged
merged 1 commit into from
Oct 24, 2021
Merged

CI: Enable overflow checks for test (non-dist) builds #89776

merged 1 commit into from
Oct 24, 2021

Conversation

hkratz
Copy link
Contributor

@hkratz hkratz commented Oct 11, 2021 •
edited
Loading

They stay disabled for Apple builds though, which take the most time already due to running on slow hw.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@hkratz
Copy link
Contributor Author

hkratz commented Oct 11, 2021

@rustbot label -S-waiting-on-review S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@hkratz
Copy link
Contributor Author

hkratz commented Oct 11, 2021

Currently fails due to #89639.

@hkratz
Copy link
Contributor Author

hkratz commented Oct 11, 2021 •
edited
Loading

@rustbot label -S-waiting-on-author S-blocked

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 11, 2021
@alex
Copy link
Member

alex commented Oct 11, 2021 via email

@hkratz hkratz mentioned this pull request Oct 12, 2021
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@hkratz

This comment has been minimized.

Sign in to view
@Mark-Simulacrum
Copy link
Member

Do we have a recent-ish measurement of how much this would slow down the compiler if enabled by default? I feel like there was a measurement a few months (years?) back...

Even if we don't enable the overflow checks within std, it seems reasonable to enable them for the compiler only, if we can afford it.

@hkratz
Copy link
Contributor Author

hkratz commented Oct 13, 2021

@Mark-Simulacrum We have measured this as part of #87784 in August. Perf results:

The slowdown is significant so to distributing rustc/std with overflow checks enabled is not a good idea. But I think it makes sense to enable the overflow checks for non-Apple non-dist CI builds. Debug assertions are enabled for those builds already and the overall impact on the CI (at least on x86_64-gnu-llvm-10 in this PR) is not that big. It avoids developer confusion encountering unexpected errors when using debug builds locally like #89639.

The slowest CI builds are the Apple ones anyway and for those we would keep it disabled (just as with debug assertions).

@Mark-Simulacrum
Copy link
Member

Thanks. Yeah, I recall those measurements now. Agreed we likely can't accept that hit just yet.

I'm fine with us merging this and we can evaluate the CI time impact if it becomes a problem, it seems reasonable to have our test builders run with more assertions where we can afford it.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 14, 2021

will also fail the tools builder due to #89280 which breaks clippy tests

@hkratz hkratz force-pushed the ci-overflow-checks branch 2 times, most recently from 383348a to 5ef1550 Compare October 16, 2021 11:05
@hkratz
Copy link
Contributor Author

hkratz commented Oct 16, 2021 •
edited
Loading

#89280 has been fixed. I have tested locally that x.py test --stage 2 and ./src/ci/docker/run.sh x86_64-gnu-tools pass with overflow checks enabled.

@rustbot label -S-blocked S-waiting-on-review

r? @Mark-Simulacrum

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Oct 16, 2021
@kennytm
Copy link
Member

kennytm commented Oct 21, 2021

@bors r=Mark-Simulacrum rollup=iffy

@bors
Copy link
Contributor

bors commented Oct 21, 2021

📌 Commit 5c8fca5 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2021
@apiraino apiraino added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Oct 21, 2021
@bors
Copy link
Contributor

bors commented Oct 23, 2021

⌛ Testing commit 5c8fca5 with merge 8ed99dd012f1305de11e33ffcb82e7f193b2feee...

@bors
Copy link
Contributor

bors commented Oct 23, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 23, 2021
@rust-log-analyzer
Copy link
Collaborator

The job i686-msvc-1 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
---- [js-doc-test] rustdoc-js\struct-like-variant.rs stdout ----

error: rustdoc-js test failed!
status: exit code: 1
command: "C:\\Program Files\\nodejs\\node" "D:\\a\\rust\\rust\\src/tools/rustdoc-js/tester.js" "--doc-folder" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc-js\\struct-like-variant" "--crate-name" "struct_like_variant" "--test-file" "D:\\a\\rust\\rust\\src/test\\rustdoc-js\\struct-like-variant.js"
------------------------------------------

------------------------------------------
stderr:
stderr:
------------------------------------------
internal/fs/utils.js:332
    throw err;


Error: ENOENT: no such file or directory, open 'D:\a\rust\rust\build\i686-pc-windows-msvc\test\rustdoc-js\struct-like-variant\search.js'
    at Object.openSync (fs.js:497:3)
    at Object.readFileSync (fs.js:393:35)
    at readFile (D:\a\rust\rust\src\tools\rustdoc-js\tester.js:181:15)
    at load_files (D:\a\rust\rust\src\tools\rustdoc-js\tester.js:391:20)
    at main (D:\a\rust\rust\src\tools\rustdoc-js\tester.js:471:27)
    at Object.<anonymous> (D:\a\rust\rust\src\tools\rustdoc-js\tester.js:491:14)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12) {
  errno: -4058,
  syscall: 'open',
  code: 'ENOENT',
  path: 'D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc-js\\struct-like-variant\\search.js'

------------------------------------------


---
test result: FAILED. 13 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.54s



command did not execute successfully: "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage0-tools-bin\\compiletest.exe" "--compile-lib-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin" "--run-lib-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\lib\\rustlib\\i686-pc-windows-msvc\\lib" "--rustc-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustc.exe" "--rustdoc-path" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage2\\bin\\rustdoc.exe" "--src-base" "D:\\a\\rust\\rust\\src/test\\rustdoc-js" "--build-base" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\test\\rustdoc-js" "--stage-id" "stage2-i686-pc-windows-msvc" "--suite" "rustdoc-js" "--mode" "js-doc-test" "--target" "i686-pc-windows-msvc" "--host" "i686-pc-windows-msvc" "--llvm-filecheck" "D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\llvm\\build\\bin\\FileCheck.exe" "--nodejs" "C:\\Program Files\\nodejs\\node" "--npm" "C:\\Program Files\\nodejs\\npm" "--host-rustcflags" "-Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--target-rustcflags" "-Cdebuginfo=0  -Lnative=D:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\native\\rust-test-helpers" "--docck-python" "C:\\hostedtoolcache\\windows\\Python\\3.10.0\\x64\\python3.exe" "--lldb-python" "C:\\hostedtoolcache\\windows\\Python\\3.10.0\\x64\\python3.exe" "--gdb" "C:\\msys64\\usr\\bin\\gdb" "--llvm-version" "13.0.0-rust-1.58.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker dwp engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink libdriver lineeditor linker lto m68k m68kasmparser m68kcodegen m68kdesc m68kdisassembler m68kinfo mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo webassemblyutils windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xray" "--cc" "" "--cxx" "" "--cflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:59:23
Build completed unsuccessfully in 0:59:23
make: *** [Makefile:72: ci-subset-1] Error 1

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Oct 23, 2021 •
edited
Loading

@bors retry - missing search.js in rustdoc-js (?) tests

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2021
@hkratz hkratz mentioned this pull request Oct 23, 2021
Closed
@bors
Copy link
Contributor

bors commented Oct 24, 2021

⌛ Testing commit 5c8fca5 with merge a99c9d6...

@bors
Copy link
Contributor

bors commented Oct 24, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing a99c9d6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 24, 2021
@bors bors merged commit a99c9d6 into rust-lang:master Oct 24, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 24, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a99c9d6): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@hkratz
Copy link
Contributor Author

hkratz commented Oct 24, 2021 •
edited
Loading

It seems that enabling this uncovers non-deterministic overflows in rustc-rayon-core causing the build failures of #90042 and #90222:

Overflow location:
https://github.com/rust-lang/rustc-rayon/blob/c8ec88d8a2236d1fe19d65a4ab38834f76d256b7/rayon-core/src/sleep/mod.rs#L330

This code is not in upstream rayon but just in our fork, introduced here:
rust-lang/rustc-rayon@27911f7#diff-cde9d726ca2f32c319420be6c61d2e57ad9daff7f85a5f8f7be25474a15f9c4dR328

@hkratz hkratz mentioned this pull request Oct 24, 2021
Open
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 24, 2021
@matthiaskrgr
ci: temporarily disable overflow checks again.
edae201 
. rust-lang#89776 enabled overflow checks in CI but these lead to two failures already:

rust-lang#90042 (comment)
rust-lang#90222 (comment)

The (first?) problem has been identified: rust-lang#90227

This PR temporarily disables the overflow checks again so we don't have to deal with the "spurious" CI failures until rustc-rayon is fixed.

Zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/ci.20failed.20with.20.20.22attempt.20to.20subtract.20with.20overflow.22
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 24, 2021
Closed
@hkratz hkratz deleted the ci-overflow-checks branch November 6, 2021 20:36
@ehuss ehuss mentioned this pull request Nov 19, 2021
Merged
@illicitonion illicitonion mentioned this pull request Nov 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.