Skip to content

Enable eager checks for memory sanitizer #99179

Closed
@nikic

Description

@nikic
Contributor

The memory sanitizer supports "eager checks", which will check initialization for noundef parameters and return values. It can be enabled by passing true as the fourth parameter to MemorySanitizerOptions. The corresponding clang option is -fsanitize-memory-param-retval. Not sure whether we need an option or can just unconditionally enable it.

Activity

added
A-LLVMArea: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.
A-sanitizersArea: Sanitizers for correctness and code quality
on Jul 12, 2022
5225225

5225225 commented on Jul 13, 2022

@5225225
Contributor

If there's not a big performance hit, and it just changes behavior, then I think we should just unconditionally enable it, saves people the hassle of needing to know to enable that flag in CI as well if you want the better checks.

In any case, I'll try and implement this, we can probably get away with not changing the rust code side at all, and just always passing EagerChecks to be true.

5225225

5225225 commented on Jul 13, 2022

@5225225
Contributor

Also, looks like we're not emitting noundef for parameters and return values, we're only doing it for loads. But if you enable optimisations, params and return values seem to get noundef (if they're of a type that we say is noundef, like char). Also, the test required #[no_mangle] on the function that created the undef value. That could just be because it's a rather artificial test case?

nikic

nikic commented on Jul 13, 2022

@nikic
ContributorAuthor

We do emit noundef attributes for params and returns -- maybe you're testing opt-level=0? Not sure which test case you are referring to here.

5225225

5225225 commented on Jul 13, 2022

@5225225
Contributor

What I don't get is how. Does noundef have no effect on unoptimised builds / is it ignored? (As in, in order to get the effect of memory sanitizer here, do you have to run an optimized build?)

I see that we have scalar_load_metadata that sets noundef metadata, is that used not just for loads, but also for parameters and returns? I can't see anywhere else that calls noundef_metadata.

#[instrument(level = "trace", skip(self))]
fn load_operand(&mut self, place: PlaceRef<'tcx, &'ll Value>) -> OperandRef<'tcx, &'ll Value> {
assert_eq!(place.llextra.is_some(), place.layout.is_unsized());
if place.layout.is_zst() {
return OperandRef::new_zst(self, place.layout);
}
#[instrument(level = "trace", skip(bx))]
fn scalar_load_metadata<'a, 'll, 'tcx>(
bx: &mut Builder<'a, 'll, 'tcx>,
load: &'ll Value,
scalar: abi::Scalar,
layout: TyAndLayout<'tcx>,
offset: Size,
) {
if !scalar.is_always_valid(bx) {
bx.noundef_metadata(load);
}
match scalar.primitive() {
abi::Int(..) => {

tmiasko

tmiasko commented on Jul 13, 2022

@tmiasko
Contributor

Attributes should be configured somewhere around here:

const OPTIMIZATION_ATTRIBUTES: [(ArgAttribute, llvm::AttributeKind); 5] = [
(ArgAttribute::NoAlias, llvm::AttributeKind::NoAlias),
(ArgAttribute::NoCapture, llvm::AttributeKind::NoCapture),
(ArgAttribute::NonNull, llvm::AttributeKind::NonNull),
(ArgAttribute::ReadOnly, llvm::AttributeKind::ReadOnly),
(ArgAttribute::NoUndef, llvm::AttributeKind::NoUndef),
];

if cx.sess().opts.optimize != config::OptLevel::No {
let deref = this.pointee_size.bytes();
if deref != 0 {
if regular.contains(ArgAttribute::NonNull) {
attrs.push(llvm::CreateDereferenceableAttr(cx.llcx, deref));
} else {
attrs.push(llvm::CreateDereferenceableOrNullAttr(cx.llcx, deref));
}
regular -= ArgAttribute::NonNull;
}
for (attr, llattr) in OPTIMIZATION_ATTRIBUTES {
if regular.contains(attr) {
attrs.push(llattr.create_attr(cx.llcx));
}
}
if regular.contains(ArgAttribute::NoAliasMutRef) && should_use_mutable_noalias(cx) {
attrs.push(llvm::AttributeKind::NoAlias.create_attr(cx.llcx));
}
}

nikic

nikic commented on Jul 13, 2022

@nikic
ContributorAuthor

What I don't get is how. Does noundef have no effect on unoptimised builds / is it ignored? (As in, in order to get the effect of memory sanitizer here, do you have to run an optimized build?)

They are not set as a compile-time optimization. It would be fine to always emit them with memory sanitizer, as they impact its behavior.

5225225

5225225 commented on Jul 13, 2022

@5225225
Contributor

Ahhh! Thank you!

So https://github.com/rust-lang/rust/blob/master/compiler/rustc_middle/src/ty/layout.rs#L3223-3226= is where noundef is given to params/return values with validity invariants. And yeah, we don't tell LLVM that anything is noundef if we have no opts on.

Maybe we should, but only if memory sanitizer is enabled? (or unconditionally?) Since it's giving msan information it needs. Not sure how many people run msan in CI and don't run it in release, but it seems non-obvious that you would need to.

added a commit that references this issue on Sep 9, 2022

Rollup merge of rust-lang#99207 - 5225225:msan-eager-checks, r=jackh726

6d20335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-LLVMArea: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.A-sanitizersArea: Sanitizers for correctness and code quality

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @nikic@5225225@tmiasko

      Issue actions

        Enable eager checks for memory sanitizer · Issue #99179 · rust-lang/rust