From 97bf80f42767ee0a7deadd883138870d2fb1ba4a Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Thu, 26 May 2016 02:45:13 +0300 Subject: [PATCH 1/6] Fix handling of C arguments Fixes #33868 --- src/librustc_trans/base.rs | 37 ++++++++++++++++++++++------------- src/librustc_trans/mir/mod.rs | 17 ++++++++++------ 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index 03e12c1c8a7b0..5a9b4e109a5ba 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -1663,21 +1663,30 @@ impl<'blk, 'tcx> FunctionContext<'blk, 'tcx> { arg_ty, datum::Lvalue::new("FunctionContext::bind_args")) } else { - unpack_datum!(bcx, datum::lvalue_scratch_datum(bcx, arg_ty, "", - uninit_reason, - arg_scope_id, |bcx, dst| { - debug!("FunctionContext::bind_args: {:?}: {:?}", hir_arg, arg_ty); + let lltmp = if common::type_is_fat_ptr(bcx.tcx(), arg_ty) { + let lltemp = alloc_ty(bcx, arg_ty, ""); let b = &bcx.build(); - if common::type_is_fat_ptr(bcx.tcx(), arg_ty) { - let meta = &self.fn_ty.args[idx]; - idx += 1; - arg.store_fn_arg(b, &mut llarg_idx, expr::get_dataptr(bcx, dst)); - meta.store_fn_arg(b, &mut llarg_idx, expr::get_meta(bcx, dst)); - } else { - arg.store_fn_arg(b, &mut llarg_idx, dst); - } - bcx - })) + // we pass fat pointers as two words, but we want to + // represent them internally as a pointer to two words, + // so make an alloca to store them in. + let meta = &self.fn_ty.args[idx]; + idx += 1; + arg.store_fn_arg(b, &mut llarg_idx, expr::get_dataptr(bcx, lltemp)); + meta.store_fn_arg(b, &mut llarg_idx, expr::get_meta(bcx, lltemp)); + lltemp + } else { + // otherwise, arg is passed by value, so store it into a temporary. + let llarg_ty = arg.cast.unwrap_or(arg.memory_ty(bcx.ccx())); + let lltemp = alloca(bcx, llarg_ty, ""); + let b = &bcx.build(); + arg.store_fn_arg(b, &mut llarg_idx, lltemp); + // And coerce the temporary into the type we expect. + b.pointercast(lltemp, arg.memory_ty(bcx.ccx()).ptr_to()) + }; + + // FIXME: hacky lol? + datum::Datum::new(lltmp, arg_ty, + datum::Lvalue::new("datum::lvalue_scratch_datum")) } } else { // FIXME(pcwalton): Reduce the amount of code bloat this is responsible for. diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index b98e04e51c007..ffc14b4468b5b 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -327,10 +327,10 @@ fn arg_value_refs<'bcx, 'tcx>(bcx: &BlockAndBuilder<'bcx, 'tcx>, llarg_idx += 1; llarg } else { - let lltemp = bcx.with_block(|bcx| { - base::alloc_ty(bcx, arg_ty, &format!("arg{}", arg_index)) - }); if common::type_is_fat_ptr(tcx, arg_ty) { + let lltemp = bcx.with_block(|bcx| { + base::alloc_ty(bcx, arg_ty, &format!("arg{}", arg_index)) + }); // we pass fat pointers as two words, but we want to // represent them internally as a pointer to two words, // so make an alloca to store them in. @@ -338,12 +338,17 @@ fn arg_value_refs<'bcx, 'tcx>(bcx: &BlockAndBuilder<'bcx, 'tcx>, idx += 1; arg.store_fn_arg(bcx, &mut llarg_idx, get_dataptr(bcx, lltemp)); meta.store_fn_arg(bcx, &mut llarg_idx, get_meta(bcx, lltemp)); + lltemp } else { - // otherwise, arg is passed by value, so make a - // temporary and store it there + // otherwise, arg is passed by value, so store it into a temporary. + let llarg_ty = arg.cast.unwrap_or(arg.memory_ty(bcx.ccx())); + let lltemp = bcx.with_block(|bcx| { + base::alloca(bcx, llarg_ty, &format!("arg{}", arg_index)) + }); arg.store_fn_arg(bcx, &mut llarg_idx, lltemp); + // And coerce the temporary into the type we expect. + bcx.pointercast(lltemp, arg.memory_ty(bcx.ccx()).ptr_to()) } - lltemp }; bcx.with_block(|bcx| arg_scope.map(|scope| { // Is this a regular argument? From b9d014b73adc34c9c19c0f6a5ae13998e46fbb6e Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Thu, 26 May 2016 02:48:25 +0300 Subject: [PATCH 2/6] Add a regression test --- .../run-pass/foreign-truncated-arguments.rs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 src/test/run-pass/foreign-truncated-arguments.rs diff --git a/src/test/run-pass/foreign-truncated-arguments.rs b/src/test/run-pass/foreign-truncated-arguments.rs new file mode 100644 index 0000000000000..a983a4a95988c --- /dev/null +++ b/src/test/run-pass/foreign-truncated-arguments.rs @@ -0,0 +1,29 @@ +// Copyright 2016 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// compile-flags: -O +// Regression test for https://github.com/rust-lang/rust/issues/33868 + +#[repr(C)] +pub struct S { + a: u32, + b: f32, + c: u32 +} + +#[no_mangle] +#[inline(never)] +pub extern "C" fn test(s: S) -> u32 { + s.c +} + +fn main() { + assert_eq!(test(S{a: 0, b: 0.0, c: 42}), 42); +} From 0d2a84c97db7969e69fd58354fea7643bde7297f Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Thu, 26 May 2016 03:13:32 +0300 Subject: [PATCH 3/6] Fix the fix/hack interaction with debuginfo --- src/librustc_trans/base.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index 5a9b4e109a5ba..f190015d02498 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -1721,16 +1721,19 @@ impl<'blk, 'tcx> FunctionContext<'blk, 'tcx> { }; let pat = &hir_arg.pat; - bcx = if let Some(name) = simple_name(pat) { - // Generate nicer LLVM for the common case of fn a pattern - // like `x: T` - set_value_name(arg_datum.val, &bcx.name(name)); - self.lllocals.borrow_mut().insert(pat.id, arg_datum); - bcx - } else { - // General path. Copy out the values that are used in the - // pattern. - _match::bind_irrefutable_pat(bcx, pat, arg_datum.match_input(), arg_scope_id) + bcx = match simple_name(pat) { + // The check for alloca is necessary because above for the immediate argument case + // we had to cast. At this point arg_datum is not an alloca anymore and thus + // breaks debuginfo if we allow this optimisation. + Some(name) + if unsafe { llvm::LLVMIsAAllocaInst(arg_datum.val) != ::std::ptr::null_mut() } => { + // Generate nicer LLVM for the common case of fn a pattern + // like `x: T` + set_value_name(arg_datum.val, &bcx.name(name)); + self.lllocals.borrow_mut().insert(pat.id, arg_datum); + bcx + }, + _ => _match::bind_irrefutable_pat(bcx, pat, arg_datum.match_input(), arg_scope_id) }; debuginfo::create_argument_metadata(bcx, hir_arg); } From e0e50a4b74c4e52e696cfe2608c3ed54a18e411f Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Thu, 26 May 2016 03:38:17 +0300 Subject: [PATCH 4/6] Fix nit/Refine the datum construction --- src/librustc_trans/base.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index f190015d02498..ddfa08516e59f 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -1683,10 +1683,8 @@ impl<'blk, 'tcx> FunctionContext<'blk, 'tcx> { // And coerce the temporary into the type we expect. b.pointercast(lltemp, arg.memory_ty(bcx.ccx()).ptr_to()) }; - - // FIXME: hacky lol? datum::Datum::new(lltmp, arg_ty, - datum::Lvalue::new("datum::lvalue_scratch_datum")) + datum::Lvalue::new("bind_args")) } } else { // FIXME(pcwalton): Reduce the amount of code bloat this is responsible for. From 2f0da79e4723f3535367581099dc59d1289a3c7c Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Thu, 26 May 2016 11:26:03 +0300 Subject: [PATCH 5/6] Do not forget to schedule the drop for the argument --- src/librustc_trans/base.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs index ddfa08516e59f..712805061ea8c 100644 --- a/src/librustc_trans/base.rs +++ b/src/librustc_trans/base.rs @@ -1660,8 +1660,8 @@ impl<'blk, 'tcx> FunctionContext<'blk, 'tcx> { self.schedule_drop_mem(arg_scope_id, llarg, arg_ty, None); datum::Datum::new(llarg, - arg_ty, - datum::Lvalue::new("FunctionContext::bind_args")) + arg_ty, + datum::Lvalue::new("FunctionContext::bind_args")) } else { let lltmp = if common::type_is_fat_ptr(bcx.tcx(), arg_ty) { let lltemp = alloc_ty(bcx, arg_ty, ""); @@ -1683,6 +1683,7 @@ impl<'blk, 'tcx> FunctionContext<'blk, 'tcx> { // And coerce the temporary into the type we expect. b.pointercast(lltemp, arg.memory_ty(bcx.ccx()).ptr_to()) }; + bcx.fcx.schedule_drop_mem(arg_scope_id, lltmp, arg_ty, None); datum::Datum::new(lltmp, arg_ty, datum::Lvalue::new("bind_args")) } From 5b404523dd1741681ba411c48c3df3c4cf5b22ab Mon Sep 17 00:00:00 2001 From: Simonas Kazlauskas Date: Thu, 26 May 2016 12:41:40 +0300 Subject: [PATCH 6/6] Fix stores codegen pass --- src/test/codegen/stores.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/codegen/stores.rs b/src/test/codegen/stores.rs index f849a6c9b18b8..8c425507975ca 100644 --- a/src/test/codegen/stores.rs +++ b/src/test/codegen/stores.rs @@ -26,8 +26,8 @@ pub struct Bytes { #[no_mangle] #[rustc_no_mir] // FIXME #27840 MIR has different codegen. pub fn small_array_alignment(x: &mut [i8; 4], y: [i8; 4]) { -// CHECK: [[VAR:%[0-9]+]] = bitcast [4 x i8]* %y to i32* -// CHECK: store i32 %{{.*}}, i32* [[VAR]], align 1 +// CHECK: store i32 %{{.*}}, i32* %{{.*}}, align 1 +// CHECK: [[VAR:%[0-9]+]] = bitcast i32* %{{.*}} to [4 x i8]* *x = y; } @@ -37,7 +37,7 @@ pub fn small_array_alignment(x: &mut [i8; 4], y: [i8; 4]) { #[no_mangle] #[rustc_no_mir] // FIXME #27840 MIR has different codegen. pub fn small_struct_alignment(x: &mut Bytes, y: Bytes) { -// CHECK: [[VAR:%[0-9]+]] = bitcast %Bytes* %y to i32* -// CHECK: store i32 %{{.*}}, i32* [[VAR]], align 1 +// CHECK: store i32 %{{.*}}, i32* %{{.*}}, align 1 +// CHECK: [[VAR:%[0-9]+]] = bitcast i32* %{{.*}} to %Bytes* *x = y; }