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.

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

libcore: Add VaList and variadic arg handling intrinsics #49878

Merged
merged 2 commits into from
Nov 29, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 185 additions & 0 deletions src/libcore/ffi.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#![stable(feature = "", since = "1.30.0")]

#![allow(non_camel_case_types)]
#![cfg_attr(stage0, allow(dead_code))]

//! Utilities related to FFI bindings.

Expand Down Expand Up @@ -40,3 +41,187 @@ impl fmt::Debug for c_void {
f.pad("c_void")
}
}

/// Basic implementation of a `va_list`.
#[cfg(any(all(not(target_arch = "aarch64"), not(target_arch = "powerpc"),
not(target_arch = "x86_64")),
windows))]
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
extern {
type VaListImpl;
}

#[cfg(any(all(not(target_arch = "aarch64"), not(target_arch = "powerpc"),
not(target_arch = "x86_64")),
windows))]
impl fmt::Debug for VaListImpl {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "va_list* {:p}", self)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with #[repr(C)] instead of #[repr(transparent)]?
And if so, can you also try extern { type VaListImpl; }?
(that would be ideal because it can't be misused like _dummy can.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although then the uninitialized in va_copy wouldn't work as well - I'm assuming the uninitialized VaListImpl in this case is unused, and LLVM overwrites the pointer itself?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could have a method on VaListImpl that takes &mut Self (in the other cases) or &() (in this case), returning &mut Self in both cases, and &mut uninitialized_variable is passed to it from copy.

In this case, you'd return &mut *(1 as *mut Self) or something like that - a dummy pointer that could be overwritten later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with #[repr(C)] instead of #[repr(transparent)]?
And if so, can you also try extern { type VaListImpl; }?
(that would be ideal because it can't be misused like _dummy can.

Trying this at the moment.

Maybe you could have a method on VaListImpl that takes &mut Self (in the other cases) or &() (in this case), returning &mut Self in both cases, and &mut uninitialized_variable is passed to it from copy.

I think this should work. I don't think it should need to take any parameters though, since self should have no impact on the uninitialized output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in all other cases you need to return a reference to an uninitialized memory location, but the pointer needs to be initialized. That's why the argument.


/// AArch64 ABI implementation of a `va_list`. See the
/// [Aarch64 Procedure Call Standard] for more details.
///
/// [AArch64 Procedure Call Standard]:
/// http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
#[cfg(all(target_arch = "aarch64", not(windows)))]
#[repr(C)]
#[derive(Debug)]
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
struct VaListImpl {
stack: *mut (),
gr_top: *mut (),
vr_top: *mut (),
gr_offs: i32,
vr_offs: i32,
}

/// PowerPC ABI implementation of a `va_list`.
#[cfg(all(target_arch = "powerpc", not(windows)))]
#[repr(C)]
#[derive(Debug)]
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
struct VaListImpl {
gpr: u8,
fpr: u8,
reserved: u16,
overflow_arg_area: *mut (),
reg_save_area: *mut (),
}

/// x86_64 ABI implementation of a `va_list`.
#[cfg(all(target_arch = "x86_64", not(windows)))]
#[repr(C)]
#[derive(Debug)]
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
struct VaListImpl {
gp_offset: i32,
fp_offset: i32,
overflow_arg_area: *mut (),
reg_save_area: *mut (),
}

/// A wrapper for a `va_list`
#[lang = "va_list"]
#[derive(Debug)]
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
#[repr(transparent)]
#[cfg(not(stage0))]
pub struct VaList<'a>(&'a mut VaListImpl);

// The VaArgSafe trait needs to be used in public interfaces, however, the trait
// itself must not be allowed to be used outside this module. Allowing users to
// implement the trait for a new type (thereby allowing the va_arg intrinsic to
// be used on a new type) is likely to cause undefined behavior.
//
// FIXME(dlrobertson): In order to use the VaArgSafe trait in a public interface
// but also ensure it cannot be used elsewhere, the trait needs to be public
// within a private module. Once RFC 2145 has been implemented look into
// improving this.
mod sealed_trait {
/// Trait which whitelists the allowed types to be used with [VaList::arg]
///
/// [VaList::va_arg]: struct.VaList.html#method.arg
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
pub trait VaArgSafe {}
}

macro_rules! impl_va_arg_safe {
($($t:ty),+) => {
$(
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
impl sealed_trait::VaArgSafe for $t {}
)+
}
}

impl_va_arg_safe!{i8, i16, i32, i64, usize}
impl_va_arg_safe!{u8, u16, u32, u64, isize}
impl_va_arg_safe!{f64}

#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
impl<T> sealed_trait::VaArgSafe for *mut T {}
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
impl<T> sealed_trait::VaArgSafe for *const T {}

#[cfg(not(stage0))]
impl<'a> VaList<'a> {
/// Advance to the next arg.
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
pub unsafe fn arg<T: sealed_trait::VaArgSafe>(&mut self) -> T {
va_arg(self)
}

/// Copy the `va_list` at the current location.
#[unstable(feature = "c_variadic",
reason = "the `c_variadic` feature has not been properly tested on \
all supported platforms",
issue = "27745")]
pub unsafe fn copy<F, R>(&mut self, f: F) -> R
where F: for<'copy> FnOnce(VaList<'copy>) -> R {
#[cfg(any(all(not(target_arch = "aarch64"), not(target_arch = "powerpc"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the cfgs here.

not(target_arch = "x86_64")),
windows))]
let mut ap = va_copy(self);
#[cfg(all(any(target_arch = "aarch64", target_arch = "powerpc", target_arch = "x86_64"),
not(windows)))]
let mut ap_inner = va_copy(self);
#[cfg(all(any(target_arch = "aarch64", target_arch = "powerpc", target_arch = "x86_64"),
not(windows)))]
let mut ap = VaList(&mut ap_inner);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be using .get_mut() on a MaybeUninit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it should be as_mut_ptr, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you actually have a reference there. Why do you want a reference to uninitialized data? So far this should be considered UB, until we have made the decision to allow it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmpf. Maybe we can #[derive(Default)] on the various definitions of VaListImpl?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or use a raw pointer in VaList.

I have no idea what this is all about, just making some guesses here.^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's semantically a reference, with a lifetime. Really, va_copy should be returning the copy instead of taking an output pointer!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you want &out or so. I guess you could emulate it with

struct VaList<'a>(*mut VaListImpl, PhantomData<&'a mut VaListImpl>)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, only va_copy does, because one of its arguments is an out pointer. All other uses want &mut. This is why I said I prefer va_copy returning the copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmpf. Maybe we can #[derive(Default)] on the various definitions of VaListImpl?

This could work. It is a type that isn't exposed to users, so I don't see a way that this could cause unintended havoc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I still prefer making va_copy return the copy, instead of having the Rust code pass a mutable reference into it.

let ret = f(VaList(ap.0));
va_end(&mut ap);
ret
}
}

#[cfg(not(stage0))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be around the whole module, in src/libcore/lib.rs.

extern "rust-intrinsic" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We loose some type safety here by using a raw pointer as the type of the input, but this makes the codegen a little less complex. We can either have a raw_mut_ptr function implemented for pointer variants of the VaList e.g. Windows, i686, etc or we can pass a &mut VaList to the intrinsics and place a conditional in the codegen that derefs the arg if the variant is a well defined structure e.g. AArch64, x86_64, etc.

/// Destroy the arglist `ap` after initialization with `va_start` or
/// `va_copy`.
fn va_end(ap: &mut VaList);

/// Copy the current location of arglist `src` to the arglist `dst`.
#[cfg(any(all(not(target_arch = "aarch64"), not(target_arch = "powerpc"),
not(target_arch = "x86_64")),
windows))]
fn va_copy<'a>(src: &VaList<'a>) -> VaList<'a>;
#[cfg(all(any(target_arch = "aarch64", target_arch = "powerpc", target_arch = "x86_64"),
not(windows)))]
fn va_copy(src: &VaList) -> VaListImpl;

/// Loads an argument of type `T` from the `va_list` `ap` and increment the
/// argument `ap` points to.
fn va_arg<T: sealed_trait::VaArgSafe>(ap: &mut VaList) -> T;
}
1 change: 1 addition & 0 deletions src/librustc/middle/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ language_item_table! {
IndexMutTraitLangItem, "index_mut", index_mut_trait, Target::Trait;

UnsafeCellTypeLangItem, "unsafe_cell", unsafe_cell_type, Target::Struct;
VaListTypeLangItem, "va_list", va_list, Target::Struct;

DerefTraitLangItem, "deref", deref_trait, Target::Trait;
DerefMutTraitLangItem, "deref_mut", deref_mut_trait, Target::Trait;
Expand Down
27 changes: 16 additions & 11 deletions src/librustc_codegen_llvm/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -723,17 +723,17 @@ impl IntrinsicDeclarationMethods<'tcx> for CodegenCx<'b, 'tcx> {
ifn!("llvm.bitreverse.i64", fn(t_i64) -> t_i64);
ifn!("llvm.bitreverse.i128", fn(t_i128) -> t_i128);

ifn!("llvm.fshl.i8", fn(t_i8, t_i8, t_i8) -> t_i8);
ifn!("llvm.fshl.i16", fn(t_i16, t_i16, t_i16) -> t_i16);
ifn!("llvm.fshl.i32", fn(t_i32, t_i32, t_i32) -> t_i32);
ifn!("llvm.fshl.i64", fn(t_i64, t_i64, t_i64) -> t_i64);
ifn!("llvm.fshl.i128", fn(t_i128, t_i128, t_i128) -> t_i128);

ifn!("llvm.fshr.i8", fn(t_i8, t_i8, t_i8) -> t_i8);
ifn!("llvm.fshr.i16", fn(t_i16, t_i16, t_i16) -> t_i16);
ifn!("llvm.fshr.i32", fn(t_i32, t_i32, t_i32) -> t_i32);
ifn!("llvm.fshr.i64", fn(t_i64, t_i64, t_i64) -> t_i64);
ifn!("llvm.fshr.i128", fn(t_i128, t_i128, t_i128) -> t_i128);
ifn!("llvm.fshl.i8", fn(t_i8, t_i8, t_i8) -> t_i8);
ifn!("llvm.fshl.i16", fn(t_i16, t_i16, t_i16) -> t_i16);
ifn!("llvm.fshl.i32", fn(t_i32, t_i32, t_i32) -> t_i32);
ifn!("llvm.fshl.i64", fn(t_i64, t_i64, t_i64) -> t_i64);
ifn!("llvm.fshl.i128", fn(t_i128, t_i128, t_i128) -> t_i128);

ifn!("llvm.fshr.i8", fn(t_i8, t_i8, t_i8) -> t_i8);
ifn!("llvm.fshr.i16", fn(t_i16, t_i16, t_i16) -> t_i16);
ifn!("llvm.fshr.i32", fn(t_i32, t_i32, t_i32) -> t_i32);
ifn!("llvm.fshr.i64", fn(t_i64, t_i64, t_i64) -> t_i64);
ifn!("llvm.fshr.i128", fn(t_i128, t_i128, t_i128) -> t_i128);

ifn!("llvm.sadd.with.overflow.i8", fn(t_i8, t_i8) -> mk_struct!{t_i8, i1});
ifn!("llvm.sadd.with.overflow.i16", fn(t_i16, t_i16) -> mk_struct!{t_i16, i1});
Expand Down Expand Up @@ -783,6 +783,11 @@ impl IntrinsicDeclarationMethods<'tcx> for CodegenCx<'b, 'tcx> {
ifn!("llvm.assume", fn(i1) -> void);
ifn!("llvm.prefetch", fn(i8p, t_i32, t_i32, t_i32) -> void);

// variadic intrinsics
ifn!("llvm.va_start", fn(i8p) -> void);
ifn!("llvm.va_end", fn(i8p) -> void);
ifn!("llvm.va_copy", fn(i8p, i8p) -> void);

if self.sess().opts.debuginfo != DebugInfo::None {
ifn!("llvm.dbg.declare", fn(self.type_metadata(), self.type_metadata()) -> void);
ifn!("llvm.dbg.value", fn(self.type_metadata(), t_i64, self.type_metadata()) -> void);
Expand Down
58 changes: 56 additions & 2 deletions src/librustc_codegen_llvm/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,14 @@ use context::CodegenCx;
use type_::Type;
use type_of::LayoutLlvmExt;
use rustc::ty::{self, Ty};
use rustc::ty::layout::{LayoutOf, HasTyCtxt};
use rustc::ty::layout::{self, LayoutOf, HasTyCtxt, Primitive};
use rustc_codegen_ssa::common::TypeKind;
use rustc::hir;
use syntax::ast;
use syntax::ast::{self, FloatTy};
use syntax::symbol::Symbol;
use builder::Builder;
use value::Value;
use va_arg::emit_va_arg;

use rustc_codegen_ssa::traits::*;

Expand Down Expand Up @@ -146,6 +147,59 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
let tp_ty = substs.type_at(0);
self.cx().const_usize(self.cx().size_of(tp_ty).bytes())
}
func @ "va_start" | func @ "va_end" => {
let va_list = match (tcx.lang_items().va_list(), &result.layout.ty.sty) {
(Some(did), ty::Adt(def, _)) if def.did == did => args[0].immediate(),
(Some(_), _) => self.load(args[0].immediate(),
tcx.data_layout.pointer_align.abi),
(None, _) => bug!("va_list language item must be defined")
};
let intrinsic = self.cx().get_intrinsic(&format!("llvm.{}", func));
self.call(intrinsic, &[va_list], None)
}
"va_copy" => {
let va_list = match (tcx.lang_items().va_list(), &result.layout.ty.sty) {
(Some(did), ty::Adt(def, _)) if def.did == did => args[0].immediate(),
(Some(_), _) => self.load(args[0].immediate(),
tcx.data_layout.pointer_align.abi),
(None, _) => bug!("va_list language item must be defined")
};
let intrinsic = self.cx().get_intrinsic(&("llvm.va_copy"));
self.call(intrinsic, &[llresult, va_list], None);
return;
}
"va_arg" => {
match fn_ty.ret.layout.abi {
layout::Abi::Scalar(ref scalar) => {
match scalar.value {
Primitive::Int(..) => {
if self.cx().size_of(ret_ty).bytes() < 4 {
// va_arg should not be called on a integer type
// less than 4 bytes in length. If it is, promote
// the integer to a `i32` and truncate the result
// back to the smaller type.
let promoted_result = emit_va_arg(self, args[0],
tcx.types.i32);
self.trunc(promoted_result, llret_ty)
} else {
emit_va_arg(self, args[0], ret_ty)
}
}
Primitive::Float(FloatTy::F64) |
Primitive::Pointer => {
emit_va_arg(self, args[0], ret_ty)
}
// `va_arg` should never be used with the return type f32.
Primitive::Float(FloatTy::F32) => {
bug!("the va_arg intrinsic does not work with `f32`")
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this special-casing here now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. I was going to leave it for when arbitrary types are supported, but I think a bug here is probably better.

}
_ => {
bug!("the va_arg intrinsic does not work with non-scalar types")
}
}
}
"size_of_val" => {
let tp_ty = substs.type_at(0);
if let OperandValue::Pair(_, meta) = args[0].val {
Expand Down
1 change: 1 addition & 0 deletions src/librustc_codegen_llvm/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ mod mono_item;
mod type_;
mod type_of;
mod value;
mod va_arg;

#[derive(Clone)]
pub struct LlvmCodegenBackend(());
Expand Down
Loading