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

Enable eager checks for memory sanitizer #99207

Merged
merged 2 commits into from
Sep 9, 2022
Merged

Enable eager checks for memory sanitizer #99207

merged 2 commits into from
Sep 9, 2022

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented Jul 13, 2022 •
edited
Loading

Fixes #99179

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 13, 2022
@rust-highfive
Copy link
Collaborator

r? @cuviper

(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 Jul 13, 2022
@5225225
Copy link
Contributor Author

5225225 commented Jul 13, 2022

Question: @nikic , were the eager checks added in LLVM 15? (So I'd add an #if LLVM_VERSION_GE(14, 0) and only pass the eager check if then?). Or was it an earlier version? I can't seem to find historical llvm docs, and https://llvm.org/doxygen/structllvm_1_1MemorySanitizerOptions.html doesn't mention the added version, so I don't know where I'd go to find out.

@nikic
Copy link
Contributor

nikic commented Jul 13, 2022

The option was added in LLVM 14.

@5225225
Copy link
Contributor Author

5225225 commented Jul 13, 2022

Strange, I was expecting that run to fail because LLVM version too old, and I'd need to put a min-llvm-version on memory-eager.rs

@5225225
Copy link
Contributor Author

5225225 commented Jul 13, 2022

Okay, I think gnu-llvm-12 doesn't have sanitizer support, so the tests were skipped. I compiled a llvm 12 locally and tested that the code worked using it. (Not having the eager checks, but it worked)

@5225225 5225225 marked this pull request as ready for review July 13, 2022 20:29
tmiasko
tmiasko reviewed Jul 16, 2022
View reviewed changes
src/test/ui/sanitize/memory.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/abi.rs Outdated Show resolved Hide resolved
@5225225 5225225 force-pushed the msan-eager-checks branch 2 times, most recently from d94eff8 to f419937 Compare July 16, 2022 11:14
@tmiasko
Copy link
Contributor

tmiasko commented Jul 16, 2022 •
edited
Loading

From the perspective of the Rust alone enabling eager checks by default sounds fine to me.

Though I wonder about mixed C/C++ builds. Clang disables eager checks by default, it also has a different understanding which arguments have undef attribute. Both cases could lead to false negatives. The side with eager checks might falsely consider the caller to be responsible for checking the undef argument and assumes the argument to be initialized.

For that reason I would lean towards exposing this functionality behind a flag and matching the clang default. What do you think?

@5225225
Copy link
Contributor Author

5225225 commented Jul 16, 2022

A LLVM person would need to answer if that's a problem.

But I do know that you get false positives if you don't have all code built with memory sanitizer, so "being careful about your dependency build flags" is already a thing.

I'm wary of making it opt-in since most code doesn't need it to be opt in.

We could add a mandatory parameter for eager checks and show a warning if the user doesn't pick one, perhaps. That'll communicate this change pretty clearly, but is rather loud.

Not sure.

@tmiasko
Copy link
Contributor

tmiasko commented Jul 16, 2022 •
edited
Loading

A LLVM person would need to answer if that's a problem.

It is a problem. It is impossible to detect uninitialized arguments of integer types, floating point types, and data pointers in calls from Rust to C when eager checks enabled in both clang and rustc. Currently the Rust side doesn't have noundef attribute for those types, so argument is not checked. The C side does have the attribute and assumes it was already checked.

There is an analogous issue when the eager checks are enabled only in rustc.

The implementation that preserves the existing default sound fine to me, if you would like to land it. With regards to the change of the default, I would wait for further support of the idea.

@cuviper
Copy link
Member

cuviper commented Jul 18, 2022

The code looks fine, but I don't have enough background in msan to judge if we should, so I'm re-rolling...

r? rust-lang/compiler

@rust-highfive rust-highfive assigned jackh726 and unassigned cuviper Jul 18, 2022
@jackh726
Copy link
Member

jackh726 commented Aug 2, 2022

I have less background to know if we should or not.

Let's cc @rust-lang/compiler

@bors
Copy link
Contributor

bors commented Aug 14, 2022

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

@jackh726
Copy link
Member

jackh726 commented Sep 5, 2022

I don't really have any background here to judge if this is a good change or not. I imagine this might need a compiler MCP, but not sure. Going to go ahead and nominate to get discussed in the compiler meeting, since that seems like to easiest way to get feedback here.

@jackh726 jackh726 added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Sep 5, 2022
@jackh726
Copy link
Member

jackh726 commented Sep 8, 2022

Talked about this in compiler meeting. Seems like we're good to just enable this.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 8, 2022

📌 Commit 16525cc has been approved by jackh726

It is now in the queue for this repository.

@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 Sep 8, 2022
@jackh726 jackh726 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. I-compiler-nominated Nominated for discussion during a compiler team meeting. labels Sep 8, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2022
@bors
Auto merge of rust-lang#101603 - matthiaskrgr:rollup-8y6kf20, r=matth…
4a09adf 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#99207 (Enable eager checks for memory sanitizer)
 - rust-lang#101253 (fix the suggestion of format for asm_sub_register)
 - rust-lang#101450 (Add `const_extern_fn` to 1.62 release notes.)
 - rust-lang#101556 (Tweak future opaque ty pretty printing)
 - rust-lang#101563 (Link UEFI target documentation from target list)
 - rust-lang#101593 (Cleanup themes (tooltip))

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6d20335 into rust-lang:master Sep 9, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 9, 2022
@5225225 5225225 deleted the msan-eager-checks branch September 9, 2022 09:35
@TimNN TimNN mentioned this pull request Sep 9, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 10, 2022
@matthiaskrgr
Rollup merge of rust-lang#101616 - TimNN:relax-msan, r=lqd
27afe55 
Adapt test for msan message change

Similar to rust-lang#100445, this adapts the new test added by rust-lang#99207 to some relatively recent [LLVM changes](llvm/llvm-project@057cabd) that removed the function name from msan messages.

Found via our experimental rust + llvm @ HEAD bot:
https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/13347#018321b2-0cc3-4c91-b4db-774477e8b074

`@rustbot` label +llvm-main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmiasko tmiasko tmiasko left review comments

Assignees

@jackh726 jackh726

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.

Enable eager checks for memory sanitizer
8 participants
@5225225 @rust-highfive @nikic @tmiasko @cuviper @jackh726 @bors @rustbot