From f75249910bb2ceff07b2924cd775333c99c2e3b5 Mon Sep 17 00:00:00 2001 From: James Miller Date: Wed, 17 Feb 2016 19:33:27 +1300 Subject: [PATCH 1/3] Handle integer-extending for C ABI We need to supply sext/zext attributes to LLVM to ensure that arguments are extended to the appropriate width in the correct way. This commit adds the necessary infrastructure for handling integer widening and makes the x86-64 code produce the correct information. --- src/librustc_llvm/lib.rs | 6 ++ src/librustc_trans/trans/cabi.rs | 18 +++++ src/librustc_trans/trans/cabi_x86_64.rs | 8 +- src/librustc_trans/trans/foreign.rs | 101 +++++++++++++++++------- 4 files changed, 101 insertions(+), 32 deletions(-) diff --git a/src/librustc_llvm/lib.rs b/src/librustc_llvm/lib.rs index 1933c926e3018..c8a40afae0152 100644 --- a/src/librustc_llvm/lib.rs +++ b/src/librustc_llvm/lib.rs @@ -261,6 +261,12 @@ impl AttrBuilder { } } + pub fn merge(&mut self, other: AttrBuilder) { + for attr in other.attrs { + self.attrs.push(attr); + } + } + pub fn arg<'a, T: AttrHelper + 'static>(&'a mut self, idx: usize, a: T) -> &'a mut AttrBuilder { self.attrs.push((idx, box a as Box)); self diff --git a/src/librustc_trans/trans/cabi.rs b/src/librustc_trans/trans/cabi.rs index 8c10be44ffde3..16ddaaf053275 100644 --- a/src/librustc_trans/trans/cabi.rs +++ b/src/librustc_trans/trans/cabi.rs @@ -29,6 +29,10 @@ pub enum ArgKind { /// Pass the argument directly using the normal converted /// LLVM type or by coercing to another specified type Direct, + /// Extend the argument for ABI requirements. The signedness + /// of the argument is required to select the appropriate + /// extension method + Extend, /// Pass the argument indirectly via a hidden pointer Indirect, /// Ignore the argument (useful for empty struct) @@ -85,6 +89,16 @@ impl ArgType { } } + pub fn extend(ty: Type) -> ArgType { + ArgType { + kind: Extend, + ty: ty, + cast: None, + pad: None, + attr: None, + } + } + pub fn is_indirect(&self) -> bool { return self.kind == Indirect; } @@ -92,6 +106,10 @@ impl ArgType { pub fn is_ignore(&self) -> bool { return self.kind == Ignore; } + + pub fn is_extend(&self) -> bool { + return self.kind == Extend; + } } /// Metadata describing how the arguments to a native function diff --git a/src/librustc_trans/trans/cabi_x86_64.rs b/src/librustc_trans/trans/cabi_x86_64.rs index 00d8fdad32de1..b99d353314a79 100644 --- a/src/librustc_trans/trans/cabi_x86_64.rs +++ b/src/librustc_trans/trans/cabi_x86_64.rs @@ -405,8 +405,12 @@ pub fn compute_abi_info(ccx: &CrateContext, None) } } else { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - ArgType::direct(ty, None, None, attr) + // Extend integer types that are < 32 bits wide + if ty.kind() == Integer && ty.int_width() < 32 { + ArgType::extend(ty) + } else { + ArgType::direct(ty, None, None, None) + } } } diff --git a/src/librustc_trans/trans/foreign.rs b/src/librustc_trans/trans/foreign.rs index b96f2cd45cf95..1cede27707c14 100644 --- a/src/librustc_trans/trans/foreign.rs +++ b/src/librustc_trans/trans/foreign.rs @@ -214,7 +214,7 @@ pub fn register_foreign_item_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, let llfn = get_extern_fn(ccx, &mut *ccx.externs().borrow_mut(), name, cc, llfn_ty, fty); attributes::unwind(llfn, false); - add_argument_attributes(&tys, llfn); + argument_attributes(&tys).apply_llfn(llfn); attributes::from_fn_attrs(ccx, attrs, llfn); llfn } @@ -367,7 +367,15 @@ pub fn trans_native_call<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, attrs.arg(1, llvm::Attribute::NoAlias) .arg(1, llvm::Attribute::NoCapture) .arg(1, llvm::DereferenceableAttribute(llret_sz)); - }; + } else if fn_type.ret_ty.is_extend() { + if let ty::FnConverging(result_ty) = fn_sig.output { + if result_ty.is_signed() { + attrs.arg(0, llvm::Attribute::SExt); + } else { + attrs.arg(0, llvm::Attribute::ZExt); + } + } + } // Add attributes that depend on the concrete foreign ABI let mut arg_idx = if fn_type.ret_ty.is_indirect() { 1 } else { 0 }; @@ -377,17 +385,26 @@ pub fn trans_native_call<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, } arg_idx += 1; - for arg_ty in &fn_type.arg_tys { - if arg_ty.is_ignore() { + for (abi_arg, arg_ty) in fn_type.arg_tys.iter().zip(fn_sig.inputs.iter()) { + if abi_arg.is_ignore() { continue; } // skip padding - if arg_ty.pad.is_some() { arg_idx += 1; } + if abi_arg.pad.is_some() { arg_idx += 1; } - if let Some(attr) = arg_ty.attr { + if let Some(attr) = abi_arg.attr { attrs.arg(arg_idx, attr); } + if abi_arg.is_extend() { + debug!("Extending Arg {}", arg_idx); + if arg_ty.is_signed() { + attrs.arg(arg_idx, llvm::Attribute::SExt); + } else { + attrs.arg(arg_idx, llvm::Attribute::ZExt); + } + } + arg_idx += 1; } @@ -549,7 +566,7 @@ pub fn decl_rust_fn_with_foreign_abi<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, }; let llfn = declare::declare_fn(ccx, name, cconv, llfn_ty, ty::FnConverging(ccx.tcx().mk_nil())); - add_argument_attributes(&tys, llfn); + argument_attributes(&tys).apply_llfn(llfn); debug!("decl_rust_fn_with_foreign_abi(llfn_ty={}, llfn={})", ccx.tn().type_to_string(llfn_ty), ccx.tn().val_to_string(llfn)); llfn @@ -572,7 +589,7 @@ pub fn register_rust_fn_with_foreign_abi(ccx: &CrateContext, let tys = foreign_types_for_fn_ty(ccx, t); let llfn_ty = lltype_for_fn_from_foreign_types(ccx, &tys); let llfn = base::register_fn_llvmty(ccx, sp, sym, node_id, cconv, llfn_ty); - add_argument_attributes(&tys, llfn); + argument_attributes(&tys).apply_llfn(llfn); debug!("register_rust_fn_with_foreign_abi(node_id={}, llfn_ty={}, llfn={})", node_id, ccx.tn().type_to_string(llfn_ty), ccx.tn().val_to_string(llfn)); llfn @@ -844,7 +861,8 @@ pub fn trans_rust_fn_with_foreign_abi<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, // Perform the call itself debug!("calling llrustfn = {}, t = {:?}", ccx.tn().val_to_string(llrustfn), t); - let attributes = attributes::from_fn_type(ccx, t); + let mut attributes = attributes::from_fn_type(ccx, t); + attributes.merge(argument_attributes(tys)); let llrust_ret_val = builder.call(llrustfn, &llrust_args, None, Some(attributes)); @@ -1024,37 +1042,60 @@ pub fn lltype_for_foreign_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, lltype_for_fn_from_foreign_types(ccx, &foreign_types_for_fn_ty(ccx, ty)) } -fn add_argument_attributes(tys: &ForeignTypes, - llfn: ValueRef) { - let mut i = if tys.fn_ty.ret_ty.is_indirect() { - 1 +fn argument_attributes(tys: &ForeignTypes) -> llvm::AttrBuilder { + let foreign_fn_ty = &tys.fn_ty; + let fn_sig = &tys.fn_sig; + + let mut attrs = llvm::AttrBuilder::new(); + + let mut idx = if foreign_fn_ty.ret_ty.is_indirect() { + 2 } else { - 0 + 1 }; - match tys.fn_ty.ret_ty.attr { - Some(attr) => unsafe { - llvm::LLVMAddFunctionAttribute(llfn, i as c_uint, attr.bits() as u64); - }, - None => {} + if let Some(attr) = foreign_fn_ty.ret_ty.attr { + if foreign_fn_ty.ret_ty.is_indirect() { + attrs.arg(1, attr); + } else { + attrs.ret(attr); + } + } + + if foreign_fn_ty.ret_ty.is_extend() { + if let ty::FnConverging(ret_ty) = fn_sig.output { + if ret_ty.is_signed() { + attrs.ret(llvm::Attribute::SExt); + } else { + attrs.ret(llvm::Attribute::ZExt); + } + } } - i += 1; + let args = foreign_fn_ty.arg_tys.iter().zip( + fn_sig.inputs.iter()); - for &arg_ty in &tys.fn_ty.arg_tys { - if arg_ty.is_ignore() { + for (abi_arg, arg_ty) in args { + if abi_arg.is_ignore() { continue; } - // skip padding - if arg_ty.pad.is_some() { i += 1; } - match arg_ty.attr { - Some(attr) => unsafe { - llvm::LLVMAddFunctionAttribute(llfn, i as c_uint, attr.bits() as u64); - }, - None => () + if abi_arg.pad.is_some() { idx += 1; } + + if let Some(attr) = abi_arg.attr { + attrs.arg(idx, attr); + } + + if abi_arg.is_extend() { + if arg_ty.is_signed() { + attrs.arg(idx, llvm::Attribute::SExt); + } else { + attrs.arg(idx, llvm::Attribute::ZExt); + } } - i += 1; + idx += 1; } + + attrs } From ad232dcf3d5fbc793b2bf43d3f7917ae93ce813a Mon Sep 17 00:00:00 2001 From: James Miller Date: Wed, 17 Feb 2016 21:02:00 +1300 Subject: [PATCH 2/3] Classify integer arguments as 'Extend' where appropriate Most platforms extend integers less than 32 bits, though not all. --- src/librustc_trans/trans/cabi.rs | 3 +-- src/librustc_trans/trans/cabi_aarch64.rs | 14 ++++++++++---- src/librustc_trans/trans/cabi_arm.rs | 14 ++++++++++---- src/librustc_trans/trans/cabi_mips.rs | 16 +++++++++++----- src/librustc_trans/trans/cabi_powerpc.rs | 16 +++++++++++----- src/librustc_trans/trans/cabi_powerpc64.rs | 14 ++++++++++---- src/librustc_trans/trans/cabi_x86.rs | 14 ++++++++++---- 7 files changed, 63 insertions(+), 28 deletions(-) diff --git a/src/librustc_trans/trans/cabi.rs b/src/librustc_trans/trans/cabi.rs index 16ddaaf053275..3ab06f0f4af3b 100644 --- a/src/librustc_trans/trans/cabi.rs +++ b/src/librustc_trans/trans/cabi.rs @@ -149,7 +149,6 @@ pub fn compute_abi_info(ccx: &CrateContext, "powerpc" => cabi_powerpc::compute_abi_info(ccx, atys, rty, ret_def), "powerpc64" => cabi_powerpc64::compute_abi_info(ccx, atys, rty, ret_def), "asmjs" => cabi_asmjs::compute_abi_info(ccx, atys, rty, ret_def), - a => ccx.sess().fatal(&format!("unrecognized arch \"{}\" in target specification", a) - ), + a => ccx.sess().fatal(&format!("unrecognized arch \"{}\" in target specification", a)), } } diff --git a/src/librustc_trans/trans/cabi_aarch64.rs b/src/librustc_trans/trans/cabi_aarch64.rs index f2434ceee2b85..7c1309fdfcb05 100644 --- a/src/librustc_trans/trans/cabi_aarch64.rs +++ b/src/librustc_trans/trans/cabi_aarch64.rs @@ -163,8 +163,11 @@ fn is_homogenous_aggregate_ty(ty: Type) -> Option<(Type, u64)> { fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - return ArgType::direct(ty, None, None, attr); + if ty.kind() == Integer && ty.int_width() < 32 { + return ArgType::extend(ty); + } else { + return ArgType::direct(ty, None, None, None); + } } if let Some((base_ty, members)) = is_homogenous_aggregate_ty(ty) { let llty = Type::array(&base_ty, members); @@ -190,8 +193,11 @@ fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { fn classify_arg_ty(ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - return ArgType::direct(ty, None, None, attr); + if ty.kind() == Integer && ty.int_width() < 32 { + return ArgType::extend(ty); + } else { + return ArgType::direct(ty, None, None, None); + } } if let Some((base_ty, members)) = is_homogenous_aggregate_ty(ty) { let llty = Type::array(&base_ty, members); diff --git a/src/librustc_trans/trans/cabi_arm.rs b/src/librustc_trans/trans/cabi_arm.rs index c5116e738048d..d423c7afd0611 100644 --- a/src/librustc_trans/trans/cabi_arm.rs +++ b/src/librustc_trans/trans/cabi_arm.rs @@ -131,8 +131,11 @@ fn ty_size(ty: Type, align_fn: TyAlignFn) -> usize { fn classify_ret_ty(ccx: &CrateContext, ty: Type, align_fn: TyAlignFn) -> ArgType { if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - return ArgType::direct(ty, None, None, attr); + if ty.kind() == Integer && ty.int_width() < 32 { + return ArgType::extend(ty); + } else { + return ArgType::direct(ty, None, None, None); + } } let size = ty_size(ty, align_fn); if size <= 4 { @@ -150,8 +153,11 @@ fn classify_ret_ty(ccx: &CrateContext, ty: Type, align_fn: TyAlignFn) -> ArgType fn classify_arg_ty(ccx: &CrateContext, ty: Type, align_fn: TyAlignFn) -> ArgType { if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - return ArgType::direct(ty, None, None, attr); + if ty.kind() == Integer && ty.int_width() < 32 { + return ArgType::extend(ty); + } else { + return ArgType::direct(ty, None, None, None); + } } let align = align_fn(ty); let size = ty_size(ty, align_fn); diff --git a/src/librustc_trans/trans/cabi_mips.rs b/src/librustc_trans/trans/cabi_mips.rs index bcffb238f5950..bb874a7f74a79 100644 --- a/src/librustc_trans/trans/cabi_mips.rs +++ b/src/librustc_trans/trans/cabi_mips.rs @@ -86,10 +86,13 @@ fn ty_size(ty: Type) -> usize { } } -fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { +fn classify_ret_ty(_ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - ArgType::direct(ty, None, None, attr) + if ty.kind() == Integer && ty.int_width() < 32 { + ArgType::extend(ty) + } else { + ArgType::direct(ty, None, None, None) + } } else { ArgType::indirect(ty, Some(Attribute::StructRet)) } @@ -105,8 +108,11 @@ fn classify_arg_ty(ccx: &CrateContext, ty: Type, offset: &mut usize) -> ArgType *offset += align_up_to(size, align * 8) / 8; if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - ArgType::direct(ty, None, None, attr) + if ty.kind() == Integer { + ArgType::extend(ty) + } else { + ArgType::direct(ty, None, None, None) + } } else { ArgType::direct( ty, diff --git a/src/librustc_trans/trans/cabi_powerpc.rs b/src/librustc_trans/trans/cabi_powerpc.rs index 1bcc8fd6bbb90..499c186eb7a3b 100644 --- a/src/librustc_trans/trans/cabi_powerpc.rs +++ b/src/librustc_trans/trans/cabi_powerpc.rs @@ -82,10 +82,13 @@ fn ty_size(ty: Type) -> usize { } } -fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { +fn classify_ret_ty(_ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - ArgType::direct(ty, None, None, attr) + if ty.kind() == Integer && ty.int_width() < 32 { + ArgType::extend(ty) + } else { + ArgType::direct(ty, None, None, None) + } } else { ArgType::indirect(ty, Some(Attribute::StructRet)) } @@ -101,8 +104,11 @@ fn classify_arg_ty(ccx: &CrateContext, ty: Type, offset: &mut usize) -> ArgType *offset += align_up_to(size, align * 8) / 8; if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - ArgType::direct(ty, None, None, attr) + if ty.kind() == Integer && ty.int_width() < 32 { + ArgType::extend(ty) + } else { + ArgType::direct(ty, None, None, None) + } } else { ArgType::direct( ty, diff --git a/src/librustc_trans/trans/cabi_powerpc64.rs b/src/librustc_trans/trans/cabi_powerpc64.rs index f76bb4f9eebc6..10f6cc5b44d44 100644 --- a/src/librustc_trans/trans/cabi_powerpc64.rs +++ b/src/librustc_trans/trans/cabi_powerpc64.rs @@ -153,8 +153,11 @@ fn is_homogenous_aggregate_ty(ty: Type) -> Option<(Type, u64)> { fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - return ArgType::direct(ty, None, None, attr); + if ty.kind() == Integer && ty.int_width() < 32 { + return ArgType::extend(ty); + } else { + return ArgType::direct(ty, None, None, None); + } } // The PowerPC64 big endian ABI doesn't return aggregates in registers @@ -187,8 +190,11 @@ fn classify_ret_ty(ccx: &CrateContext, ty: Type) -> ArgType { fn classify_arg_ty(ccx: &CrateContext, ty: Type) -> ArgType { if is_reg_ty(ty) { - let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - return ArgType::direct(ty, None, None, attr); + if ty.kind() == Integer && ty.int_width() < 32 { + return ArgType::extend(ty); + } else { + return ArgType::direct(ty, None, None, None); + } } if let Some((base_ty, members)) = is_homogenous_aggregate_ty(ty) { let llty = Type::array(&base_ty, members); diff --git a/src/librustc_trans/trans/cabi_x86.rs b/src/librustc_trans/trans/cabi_x86.rs index 50a3095dea169..95bfa7036e894 100644 --- a/src/librustc_trans/trans/cabi_x86.rs +++ b/src/librustc_trans/trans/cabi_x86.rs @@ -56,8 +56,11 @@ pub fn compute_abi_info(ccx: &CrateContext, } } } else { - let attr = if rty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - ret_ty = ArgType::direct(rty, None, None, attr); + ret_ty = if rty.kind() == Integer && rty.int_width() < 32 { + ArgType::extend(rty) + } else { + ArgType::direct(rty, None, None, None) + }; } for &t in atys { @@ -71,8 +74,11 @@ pub fn compute_abi_info(ccx: &CrateContext, } } _ => { - let attr = if t == Type::i1(ccx) { Some(Attribute::ZExt) } else { None }; - ArgType::direct(t, None, None, attr) + if t.kind() == Integer && t.int_width() < 32 { + ArgType::extend(t) + } else { + ArgType::direct(t, None, None, None) + } } }; arg_tys.push(ty); From 3f669e6c2e7036f13c3d720037fb646ce59f59c1 Mon Sep 17 00:00:00 2001 From: James Miller Date: Wed, 17 Feb 2016 22:34:15 +1300 Subject: [PATCH 3/3] Add test for C ABI integer widening --- src/test/run-make/cabi-int-widening/Makefile | 10 +++++++++ src/test/run-make/cabi-int-widening/cfoo.c | 4 ++++ src/test/run-make/cabi-int-widening/foo.rs | 22 ++++++++++++++++++++ 3 files changed, 36 insertions(+) create mode 100644 src/test/run-make/cabi-int-widening/Makefile create mode 100644 src/test/run-make/cabi-int-widening/cfoo.c create mode 100644 src/test/run-make/cabi-int-widening/foo.rs diff --git a/src/test/run-make/cabi-int-widening/Makefile b/src/test/run-make/cabi-int-widening/Makefile new file mode 100644 index 0000000000000..7ea2586186862 --- /dev/null +++ b/src/test/run-make/cabi-int-widening/Makefile @@ -0,0 +1,10 @@ +-include ../tools.mk + +all: $(call NATIVE_STATICLIB,cfoo) + $(RUSTC) foo.rs + $(call RUN,foo) + +# Need to optimize the C code so it actually takes advantage +# of the ABI semantics +$(TMPDIR)/libcfoo.o: cfoo.c + $(call COMPILE_OBJ,$(TMPDIR)/libcfoo.o,cfoo.c -O) diff --git a/src/test/run-make/cabi-int-widening/cfoo.c b/src/test/run-make/cabi-int-widening/cfoo.c new file mode 100644 index 0000000000000..cb7aca6f28114 --- /dev/null +++ b/src/test/run-make/cabi-int-widening/cfoo.c @@ -0,0 +1,4 @@ +// ignore-license +int foo(char x) { + return (int)x; +} diff --git a/src/test/run-make/cabi-int-widening/foo.rs b/src/test/run-make/cabi-int-widening/foo.rs new file mode 100644 index 0000000000000..f954532f65cae --- /dev/null +++ b/src/test/run-make/cabi-int-widening/foo.rs @@ -0,0 +1,22 @@ +// Copyright 2014 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. + +#[link(name = "cfoo")] +extern { + fn foo(x: i8) -> i32; +} + +fn main() { + let x = unsafe { + foo(-1) + }; + + assert!(x == -1); +}