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

Aarch64 call abi does not zeroext (and one cannot assume it does so) #97800

Merged
41 changes: 34 additions & 7 deletions compiler/rustc_target/src/abi/call/aarch64.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,27 @@
use crate::abi::call::{ArgAbi, FnAbi, Reg, RegKind, Uniform};
use crate::abi::{HasDataLayout, TyAbiInterface};

/// Given integer-types M and register width N (e.g. M=u16 and N=32 bits), the
/// `ParamExtension` policy specifies how a uM value should be treated when
/// passed via register or stack-slot of width N. See also rust-lang/rust#97463.
#[derive(Copy, Clone, PartialEq)]
pub enum ParamExtension {
/// Indicates that when passing an i8/i16, either as a function argument or
/// as a return value, it must be sign-extended to 32 bits, and likewise a
/// u8/u16 must be zero-extended to 32-bits. (This variant is here to
/// accommodate Apple's deviation from the usual AArch64 ABI as defined by
/// ARM.)
///
/// See also: <https://developer.apple.com/documentation/xcode/writing-arm64-code-for-apple-platforms#Pass-Arguments-to-Functions-Correctly>
ExtendTo32Bits,

/// Indicates that no sign- nor zero-extension is performed: if a value of
/// type with bitwidth M is passed as function argument or return value,
/// then M bits are copied into the least significant M bits, and the
/// remaining bits of the register (or word of memory) are untouched.
NoExtension,
}

fn is_homogeneous_aggregate<'a, Ty, C>(cx: &C, arg: &mut ArgAbi<'a, Ty>) -> Option<Uniform>
where
Ty: TyAbiInterface<'a, C> + Copy,
Expand All @@ -24,13 +45,16 @@ where
})
}

fn classify_ret<'a, Ty, C>(cx: &C, ret: &mut ArgAbi<'a, Ty>)
fn classify_ret<'a, Ty, C>(cx: &C, ret: &mut ArgAbi<'a, Ty>, param_policy: ParamExtension)
where
Ty: TyAbiInterface<'a, C> + Copy,
C: HasDataLayout,
{
if !ret.layout.is_aggregate() {
ret.extend_integer_width_to(32);
pnkfelix marked this conversation as resolved.
Show resolved Hide resolved
match param_policy {
ParamExtension::ExtendTo32Bits => ret.extend_integer_width_to(32),
ParamExtension::NoExtension => {}
}
return;
}
if let Some(uniform) = is_homogeneous_aggregate(cx, ret) {
Expand All @@ -46,13 +70,16 @@ where
ret.make_indirect();
}

fn classify_arg<'a, Ty, C>(cx: &C, arg: &mut ArgAbi<'a, Ty>)
fn classify_arg<'a, Ty, C>(cx: &C, arg: &mut ArgAbi<'a, Ty>, param_policy: ParamExtension)
where
Ty: TyAbiInterface<'a, C> + Copy,
C: HasDataLayout,
{
if !arg.layout.is_aggregate() {
arg.extend_integer_width_to(32);
match param_policy {
ParamExtension::ExtendTo32Bits => arg.extend_integer_width_to(32),
ParamExtension::NoExtension => {}
}
Comment on lines +79 to +82
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I wonder if we should pull this logic into a method on ParamExtension instead of duplicating it at these two places?

return;
}
if let Some(uniform) = is_homogeneous_aggregate(cx, arg) {
Expand All @@ -68,19 +95,19 @@ where
arg.make_indirect();
}

pub fn compute_abi_info<'a, Ty, C>(cx: &C, fn_abi: &mut FnAbi<'a, Ty>)
pub fn compute_abi_info<'a, Ty, C>(cx: &C, fn_abi: &mut FnAbi<'a, Ty>, param_policy: ParamExtension)
where
Ty: TyAbiInterface<'a, C> + Copy,
C: HasDataLayout,
{
if !fn_abi.ret.is_ignore() {
classify_ret(cx, &mut fn_abi.ret);
classify_ret(cx, &mut fn_abi.ret, param_policy);
}

for arg in &mut fn_abi.args {
if arg.is_ignore() {
continue;
}
classify_arg(cx, arg);
classify_arg(cx, arg, param_policy);
}
}
9 changes: 8 additions & 1 deletion compiler/rustc_target/src/abi/call/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,14 @@ impl<'a, Ty> FnAbi<'a, Ty> {
}
}
},
"aarch64" => aarch64::compute_abi_info(cx, self),
"aarch64" => {
let param_policy = if cx.target_spec().is_like_osx {
aarch64::ParamExtension::ExtendTo32Bits
} else {
aarch64::ParamExtension::NoExtension
};
aarch64::compute_abi_info(cx, self, param_policy)
}
"amdgpu" => amdgpu::compute_abi_info(cx, self),
"arm" => arm::compute_abi_info(cx, self),
"avr" => avr::compute_abi_info(self),
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_target/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1250,6 +1250,8 @@ pub struct TargetOptions {
pub abi_return_struct_as_int: bool,
/// Whether the target toolchain is like macOS's. Only useful for compiling against iOS/macOS,
/// in particular running dsymutil and some other stuff like `-dead_strip`. Defaults to false.
/// Also indiates whether to use Apple-specific ABI changes, such as extending function
/// parameters to 32-bits.
pub is_like_osx: bool,
/// Whether the target toolchain is like Solaris's.
/// Only useful for compiling against Illumos/Solaris,
Expand Down
12 changes: 12 additions & 0 deletions src/test/auxiliary/rust_test_helpers.c
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Helper functions used only in tests

#include <stdint.h>
#include <stdlib.h>
#include <assert.h>
#include <stdarg.h>

Expand Down Expand Up @@ -415,3 +416,14 @@ rust_dbg_unpack_option_u64u64(struct U8TaggedEnumOptionU64U64 o, uint64_t *a, ui
return 0;
}
}

uint16_t issue_97463_leak_uninit_data(uint32_t a, uint32_t b, uint32_t c) {
struct bloc { uint16_t a; uint16_t b; uint16_t c; };
struct bloc *data = malloc(sizeof(struct bloc));

data->a = a & 0xFFFF;
data->b = b & 0xFFFF;
data->c = c & 0xFFFF;

return data->b; /* leak data */
}
46 changes: 44 additions & 2 deletions src/test/codegen/abi-repr-ext.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,56 @@
// compile-flags: -O

#![crate_type="lib"]
// revisions:x86_64 i686 aarch64-apple aarch64-windows aarch64-linux arm riscv

//[x86_64] compile-flags: --target x86_64-unknown-uefi
//[x86_64] needs-llvm-components: x86
//[i686] compile-flags: --target i686-unknown-linux-musl
//[i686] needs-llvm-components: x86
//[aarch64-windows] compile-flags: --target aarch64-pc-windows-msvc
//[aarch64-windows] needs-llvm-components: aarch64
//[aarch64-linux] compile-flags: --target aarch64-unknown-linux-gnu
//[aarch64-linux] needs-llvm-components: aarch64
//[aarch64-apple] compile-flags: --target aarch64-apple-darwin
//[aarch64-apple] needs-llvm-components: aarch64
//[arm] compile-flags: --target armv7r-none-eabi
//[arm] needs-llvm-components: arm
//[riscv] compile-flags: --target riscv64gc-unknown-none-elf
//[riscv] needs-llvm-components: riscv

// See bottom of file for a corresponding C source file that is meant to yield
// equivalent declarations.
#![feature(no_core, lang_items)]
#![crate_type = "lib"]
#![no_std]
#![no_core]

#[lang="sized"] trait Sized { }
#[lang="freeze"] trait Freeze { }
#[lang="copy"] trait Copy { }

#[repr(i8)]
pub enum Type {
Type1 = 0,
Type2 = 1
}

// CHECK: define{{( dso_local)?}} noundef signext i8 @test()
// To accommodate rust#97800, one might consider writing the below as:
//
// `define{{( dso_local)?}} noundef{{( signext)?}} i8 @test()`
//
// but based on rust#80556, it seems important to actually check for the
// presence of the `signext` for those targets where we expect it.

// CHECK: define{{( dso_local)?}} noundef
// x86_64-SAME: signext
// aarch64-apple-SAME: signext
// aarch64-windows-NOT: signext
// aarch64-linux-NOT: signext
// arm-SAME: signext
// riscv-SAME: signext
// CHECK-SAME: i8 @test()


#[no_mangle]
pub extern "C" fn test() -> Type {
Type::Type1
Expand Down
5 changes: 4 additions & 1 deletion src/test/codegen/pic-relocation-model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ pub fn call_foreign_fn() -> u8 {
}
}

// CHECK: declare zeroext i8 @foreign_fn()
// (Allow but do not require `zeroext` here, because it is not worth effort to
// spell out which targets have it and which ones do not; see rust#97800.)

// CHECK: declare{{( zeroext)?}} i8 @foreign_fn()
extern "C" {fn foreign_fn() -> u8;}

// CHECK: !{i32 7, !"PIC Level", i32 2}
Loading