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

Test that core can be built with msan enabled #79382

Closed
wants to merge 1 commit into from
Closed

Test that core can be built with msan enabled #79382

wants to merge 1 commit into from

Conversation

g2p
Copy link
Contributor

@g2p g2p commented Nov 24, 2020

See rust-fuzz/cargo-fuzz#243

The test currently fails, since it highlights a regression in https://github.com/rust-lang/stdarch.

Reverting library/stdarch to rust-lang/stdarch@718175b
(the problem commit being rust-lang/stdarch@3fa0f6a)
makes it pass again.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Nov 24, 2020
@g2p g2p mentioned this pull request Nov 24, 2020
Open
@camelid
Copy link
Member

camelid commented Nov 25, 2020 •
edited
Loading

The test currently fails, since it highlights a regression in https://github.com/rust-lang/stdarch.

Hmm, CI passed though. Maybe CI is skipping the test because it doesn't have msan or something?

@camelid camelid added A-sanitizers Area: Sanitizers for correctness and code quality T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 25, 2020
@g2p
Copy link
Contributor Author

g2p commented Nov 25, 2020

The test currently fails, since it highlights a regression in https://github.com/rust-lang/stdarch.

Hmm, CI passed though. Maybe CI is skipping the test because it doesn't have msan or something?

Indeed, src/ci/docker/host-x86_64/x86_64-gnu-llvm-9/Dockerfile does not pass --enable-sanitizers in RUST_CONFIGURE_ARGS.

It may be worth adding, after the underlying issue is fixed.

@g2p
Copy link
Contributor Author

g2p commented Nov 26, 2020

Getting the test to pass will require at least fixing this LLVM issue:
https://bugs.llvm.org/show_bug.cgi?id=48298

Is there a way to allow the test to fail so it can be merged?
Or I might use min-llvm-version with a high version?

@camelid
Copy link
Member

camelid commented Nov 26, 2020

Using // ignore at the top might work, though I'm not sure. Probably worth a try though!

@g2p g2p mentioned this pull request Nov 27, 2020
Merged
@Mark-Simulacrum
Copy link
Member

I would prefer to not add an ignored test at this time, I think that's likely to just get forgotten. If you want to take the test and file an issue for this bug, though, that would work and we can add it once the bug is fixed.

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

@g2p Ping from triage, that llvm update seems landed. Would you mind rebasing so we can try again? Don't forget to switch the PR state to S-waiting-on-review when it's ready. Thanks!

@camelid
Copy link
Member

camelid commented Dec 18, 2020

Note: to change the labels, post this in a comment: @rustbot label: +S-waiting-on-review -S-waiting-on-author

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

@g2p Ping from triage, can you post your status on this pull request? Is it ready for review?

@g2p
Test that core can be built with msan enabled
440554e 
See rust-fuzz/cargo-fuzz#243

The test currently fails, since it highlights a regression.

Reverting library/stdarch to <rust-lang/stdarch@718175b>
(the problem commit being <rust-lang/stdarch@3fa0f6a>)
makes it pass again.
@g2p g2p force-pushed the test-msan-build-core branch from 786cea1 to 440554e Compare January 12, 2021 10:29
@g2p
Copy link
Contributor Author

g2p commented Jan 12, 2021

LLVM updates didn't seem sufficient last time I tried cherry-picking, but now they are, and the test passes.
I'll update the docker config to enable sanitizers, then wait for the bots.

@Mark-Simulacrum
Copy link
Member

Does the test still fail? I don't see this updating docker configs I guess...

I suspect we'll need to pull through a $CARGO env var into the test somehow, I don't think our CI environment can rely on having it in PATH.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 16, 2021
@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2021
@Dylan-DPC-zz
Copy link

@g2p any updates on this?

@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Mar 1, 2021
@JohnCSimon
Copy link
Member

@g2p
this hasn't seen any movement in the last month and a half, so I'm closing this as inactive.
Please feel free to reopen when you can continue.

@JohnCSimon JohnCSimon closed this Mar 1, 2021
This was referenced Oct 31, 2024
Closed
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
A-sanitizers Area: Sanitizers for correctness and code quality S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@g2p @rust-highfive @camelid @Mark-Simulacrum @crlf0710 @JohnCSimon @Dylan-DPC-zz @rustbot