llvm lint: Undefined behavior: Memory reference address is misaligned

[matthiaskrgr]

reduced from num_cpus:

extern crate libc;

#[inline]
pub fn get_physical() -> usize {
    get_num_physical_cpus()
}

#[cfg(target_os = "linux")]
fn get_num_physical_cpus() -> usize {
    use std::collections::HashSet;
    use std::fs::File;
    use std::io::BufRead;
    use std::io::BufReader;

    let file = File::open("/proc/cpuinfo").unwrap();
    let reader = BufReader::new(file);
    let mut set = HashSet::new();
    for _ in reader.lines().filter_map(|result| result.ok()) {
        set.insert((0, 0));
    }
    4
}

built with
RUSTFLAGS="-C passes=lint" cargo build --release -j 1 yields

   Compiling libc v0.2.44
Unusual: Return statement in function with noreturn attribute
  ret void
   Compiling num_cpus v1.8.0 (/tmp/num_cpus)
Undefined behavior: Memory reference address is misaligned
  %61 = load i32, i32* %60, align 8
Undefined behavior: Memory reference address is misaligned
  %132 = load i32, i32* %131, align 8
    Finished release [optimized] target(s) in 1.12s

cc #7463

rustc 1.32.0-nightly (6bfb46e4a 2018-11-26)
binary: rustc
commit-hash: 6bfb46e4ac9a2704f06de1a2ff7a4612cd70c8cb
commit-date: 2018-11-26
host: x86_64-unknown-linux-gnu
release: 1.32.0-nightly
LLVM version: 8.0

[nagisa]

What’s the target? Ah, never mind, I missed that its x86_64-unknown-linux-gnu.

[nagisa]

More minimal:

// rustc file.rs -O -Cpasses=lint --crate-type=rlib
pub fn get_physical() {
    get_num_physical_cpus()
}

fn get_num_physical_cpus() {
    use std::collections::HashMap;
    let mut set = HashMap::new();
    set.insert((0, 0), ());
}

Issue does not occur on stable (or 1.30.0 nightly), but occurs on rustc beta.

[nagisa]

Marking T-libs as that is likely to be an issue within implementation of HashMap, but I didn’t investigate further.

[nikic]

Excerpt:

; <std::collections::hash::map::InternalEntry<K, V, &'a mut std::collections::hash::table::RawTable<K, V>>>::into_entry

  %40 = getelementptr inbounds %"std::collections::hash::map::VacantEntry<(i32, i32), ()>", %"std::collections::hash::map::VacantEntry<(i32, i32), ()>"* %_13, i32 0, i32 5
  %41 = getelementptr inbounds { i32, i32 }, { i32, i32 }* %40, i32 0, i32 0
  store i32 %key.0, i32* %41, align 8
  %42 = getelementptr inbounds { i32, i32 }, { i32, i32 }* %40, i32 0, i32 1
  store i32 %key.1, i32* %42, align 8

Clearly both of those adjacent i32 aren't going to be 8-byte aligned ^^ It seems that we're storing to an (i32, i32) key with wrong alignment for some reason.

[nikic]

Don't need to look particularly far, this code

pub struct Foo<T> {
    foo: u64,
    bar: T,
}

pub fn test(x: (i32, i32)) -> Foo<(i32, i32)> {
    Foo { foo: 0, bar: x }
}

generates

%"Foo<(i32, i32)>" = type { [0 x i64], i64, [0 x i32], { i32, i32 }, [0 x i32] }

; file2::test
; Function Attrs: nonlazybind uwtable
define void @_ZN5file24test17h241867b5d90e9ca9E(%"Foo<(i32, i32)>"* noalias nocapture sret dereferenceable(16), i32 %x.0, i32 %x.1) unnamed_addr #0 {
start:
  %1 = bitcast %"Foo<(i32, i32)>"* %0 to i64*
  store i64 0, i64* %1, align 8
  %2 = getelementptr inbounds %"Foo<(i32, i32)>", %"Foo<(i32, i32)>"* %0, i32 0, i32 3
  %3 = getelementptr inbounds { i32, i32 }, { i32, i32 }* %2, i32 0, i32 0
  store i32 %x.0, i32* %3, align 8
  %4 = getelementptr inbounds { i32, i32 }, { i32, i32 }* %2, i32 0, i32 1
  store i32 %x.1, i32* %4, align 8
  ret void
}

It looks like some code is assuming that both elements in a scalar pair have the same alignment as the whole pair.

[nagisa]

Probably the easiest way to get to the root cause is to bisect.

[nikic]

This code is very likely the culprit:

rust/src/librustc_codegen_ssa/mir/operand.rs

Lines 333 to 339 in 400c2bc

	OperandValue::Pair(a, b) => {
	for (i, &x) in [a, b].iter().enumerate() {
	let llptr = bx.struct_gep(dest.llval, i as u64);
	let val = base::from_immediate(bx, x);
	bx.store_with_flags(val, llptr, dest.align, flags);
	}
	}

It takes the 0 and 1 GEPs, but uses the same alignment, without offset adjustment.

[eddyb]

Ouch, that's bad. To obtain the alignment for both fields, we need to do:

let (a_scalar, b_scalar) = match dest.layout.abi {
    layout::Abi::ScalarPair(ref a, ref b) => (a, b),
    _ => bug!("store_with_flags: invalid ScalarPair layout: {:#?}", layout)
};
// This has been copy-pasted all over the place, I wonder how to deduplicate it
let b_offset = a_scalar.value.size(bx).align_to(b_scalar.value.align(bx).abi);

let a_align = dest.align;
let b_align = dest.align.restrict_for_offset(b_offset);

Note that we can't use project_field because the ScalarPair components do not match fields (the latter come from the user type definitions, whereas the former are extracted as an optimization).