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.

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

Re-enable debug and LLVM assertions #75199

Merged
merged 2 commits into from
Nov 7, 2020

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Aug 5, 2020

Historically we've disabled these assertions on a number of platforms with the
goal of speeding up CI. Now, though, having migrated to GitHub actions, CI is
already pretty fast, and these debug assertions do bring us some value.

This does leave in some debug assertions that are performance-related: macOS
currently hovers at just under 2 hours.

There are also some other builders which have debug and LLVM assertions
disabled:

llvm-8, PR builder:

In one view, this builder tests our support for older LLVMs. But in reality, a
lot of our tests already disable themselves on older LLVMs, and I think our
general stance is that we really only support the in-tree LLVM. Plus, we really
want CI times on this builder to be really low, as it's run on every PR --
that's a lot of CI time.

test-various:

This disables debug asserts still -- as noted in the Dockerfile, we test code
size, and we need debug asserts off for that to work well.

Helps with #59637 -- but doesn't close it, macOS still has asserts off.

r? @pietroalbini

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 5, 2020
@Mark-Simulacrum
Copy link
Member Author

@nikic @cuviper (I guess we need an LLVM alias that's more "team" oriented rather than the ping group):

Do you agree that my interpretation of the llvm-8 builder is accurate? Should we be running debug asserts on it on the auto branch perhaps?

@cuviper
Copy link
Member

cuviper commented Aug 5, 2020

llvm-8, PR builder:

In one view, this builder tests our support for older LLVMs. But in reality, a
lot of our tests already disable themselves on older LLVMs, and I think our
general stance is that we really only support the in-tree LLVM.

Well, you already know that external LLVM support is important to me for distro needs. I think this builder is useful as a sanity check, both that external LLVM works and that the stated minimum is explicitly maintained. That's true even without full testing -- just the fact that it bootstraps successfully will catch a lot.

The setting for LLVM assertions is irrelevant there, since it's not building LLVM. Rust debug assertions are debatable, but I agree it's nice to keep this fast for first-line pull request CI. Do you know how much that would slow it down?

@Mark-Simulacrum
Copy link
Member Author

Mark-Simulacrum commented Aug 5, 2020

That's certainly true. I didn't intend to imply that external LLVM support isn't needed, more so that super recent LLVM is what is guaranteed to work -- we've had soundness fixes and such I think by way of LLVM patches, upstreamed. Maybe I'm misremembering that though.

It's a good point though that since we're not building LLVM the assertions setting doesn't matter.

I don't know how much of a slow down debug assertions would be on the PR builder -- I can try and test that a bit, I guess, just rerunning the builder on this PR a few times after pushing up an amendment. I'll do that soon.


Using as a record of timings so far:

  • Without asserts: 33m, 33m
  • With asserts: 42m, 43m, 44m, 37m, 41m, 42m, 37m

@pietroalbini
Copy link
Member

@Mark-Simulacrum what about disabling assertions in PR llvm-8, and enabling them in auto llvm-8?

@Mark-Simulacrum
Copy link
Member Author

Yeah I'd rather not diverge the builders like that -- makes it even more annoying to debug auto builder failures if we have differing behaviors between PR and not PR.

@nikic
Copy link
Contributor

nikic commented Aug 9, 2020

FWIW I don't think debug assertions are particularly useful for the LLVM 8 builder -- having LLVM assertions there would be useful, but not really possible with distro packages. Having debug assertions there would only catch the pretty narrow case of an older LLVM version miscompiling code inside a debug assertion specifically...

@cuviper
Copy link
Member

cuviper commented Aug 9, 2020

It's not useful for LLVM's sake, but debug assertions may still catch if a pull request introduces a bug in general, and it's nice if we can do that before the review and full CI.

The LLVM 8 builder is chosen for this just because it runs automatically, since it's faster without building LLVM itself.

@jyn514 jyn514 added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 9, 2020
@Mark-Simulacrum
Copy link
Member Author

So adding debug assertions to the PR builder seems like a 6-8 minute hit (roughly 33 to 40m). I don't really care much either way.

@bors
Copy link
Contributor

bors commented Aug 14, 2020

☔ The latest upstream changes (presumably #73383) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon 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 Sep 1, 2020
@pietroalbini
Copy link
Member

I'm ok with the current changes, I agree that testing assertions on llvm-8 is not crucial. r=me once the merge conflict is fixed.

@Mark-Simulacrum Mark-Simulacrum force-pushed the debug-asserts branch 2 times, most recently from f6827fe to 2ca5d0d Compare September 3, 2020 13:10
@Mark-Simulacrum
Copy link
Member Author

@bors r=pietroalbini

@bors
Copy link
Contributor

bors commented Sep 3, 2020

📌 Commit 2ca5d0d150d9da70b57c279a72255812d3ef6d2b has been approved by pietroalbini

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 3, 2020
@bors
Copy link
Contributor

bors commented Sep 4, 2020

⌛ Testing commit 2ca5d0d150d9da70b57c279a72255812d3ef6d2b with merge eaccf682adaf879b494c8bee7363b5b83fcb1ffb...

@bors
Copy link
Contributor

bors commented Sep 4, 2020

💔 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 Sep 4, 2020
@pietroalbini pietroalbini 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 Sep 4, 2020
Historically we've disabled these assertions on a number of platforms with the
goal of speeding up CI. Now, though, having migrated to GitHub actions, CI is
already pretty fast, and these debug assertions do bring us some value.

This does leave in some debug assertions that are performance-related: macOS
currently hovers at just under 2 hours.

There are also some other builders which have debug and LLVM assertions
disabled:

llvm-8, PR builder:

In one view, this builder tests our support for older LLVMs. But in reality, a
lot of our tests already disable themselves on older LLVMs, and I think our
general stance is that we really only support the in-tree LLVM. Plus, we really
want CI times on this builder to be really low, as it's run on *every* PR --
that's a lot of CI time.

test-various:

This disables debug asserts still -- as noted in the Dockerfile, we test code
size, and we need debug asserts off for that to work well.
This is helpful to catch slightly more bugs before things hit main CI, and
doesn't cost too much extra CI time.
@Mark-Simulacrum
Copy link
Member Author

My hope is that this will now just pass (with #76859 merged).

@bors r=pietroalbini rollup=never

@bors
Copy link
Contributor

bors commented Nov 6, 2020

📌 Commit 3ca8829 has been approved by pietroalbini

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 6, 2020
@bors
Copy link
Contributor

bors commented Nov 6, 2020

⌛ Testing commit 3ca8829 with merge 8e20c1e26dd4438a133e5a99acc7b4ef0c5cb958...

@bors
Copy link
Contributor

bors commented Nov 7, 2020

💥 Test timed out

@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 Nov 7, 2020
@Mark-Simulacrum
Copy link
Member Author

@bors retry

AFAICT, the powerpc dist builder is still running, but this PR doesn't touch it to my knowledge.

@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 Nov 7, 2020
@bors
Copy link
Contributor

bors commented Nov 7, 2020

⌛ Testing commit 3ca8829 with merge d6f4abe8ca3ed385639ed9bbe6c10e56b688f0d9...

@bors
Copy link
Contributor

bors commented Nov 7, 2020

💔 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 Nov 7, 2020
@Mark-Simulacrum
Copy link
Member Author

2020-11-07T05:28:32.4615266Z FAILED: sccache /usr/bin/c++   -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/ProfileData -I/checkout/src/llvm-project/llvm/lib/ProfileData -Iinclude -I/checkout/src/llvm-project/llvm/include -ffunction-sections -fdata-sections -fPIC -m64 -static-libstdc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG    -fno-exceptions -fno-rtti -std=c++14 -MMD -MT lib/ProfileData/CMakeFiles/LLVMProfileData.dir/GCOV.cpp.o -MF lib/ProfileData/CMakeFiles/LLVMProfileData.dir/GCOV.cpp.o.d -o lib/ProfileData/CMakeFiles/LLVMProfileData.dir/GCOV.cpp.o -c /checkout/src/llvm-project/llvm/lib/ProfileData/GCOV.cpp
2020-11-07T05:28:32.4624500Z <built-in>: internal compiler error: Segmentation fault
2020-11-07T05:28:32.4625139Z Please submit a full bug report,
2020-11-07T05:28:32.4625673Z with preprocessed source if appropriate.
2020-11-07T05:28:32.4627542Z See <file:///usr/share/doc/gcc-5/README.Bugs> for instructions.
2020-11-07T05:28:37.8020856Z ninja: build stopped: subcommand failed.

Seems spurious, @bors retry

@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 Nov 7, 2020
@bors
Copy link
Contributor

bors commented Nov 7, 2020

⌛ Testing commit 3ca8829 with merge b2d115f...

@bors
Copy link
Contributor

bors commented Nov 7, 2020

☀️ Test successful - checks-actions
Approved by: pietroalbini
Pushing b2d115f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 7, 2020
@bors bors merged commit b2d115f into rust-lang:master Nov 7, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.