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
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
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 */
}
204 changes: 204 additions & 0 deletions src/test/codegen/some-abis-do-extend-params-to-32-bits.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
// compile-flags: -Cno-prepopulate-passes

// 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 { }

// The patterns in this file are written in the style of a table to make the
// uniformities and distinctions more apparent.
//
// ZERO/SIGN-EXTENDING TO 32 BITS NON-EXTENDING
// ============================== =======================
// x86_64: void @c_arg_u8(i8 zeroext %_a)
// i686: void @c_arg_u8(i8 zeroext %_a)
// aarch64-apple: void @c_arg_u8(i8 zeroext %_a)
// aarch64-windows: void @c_arg_u8(i8 %_a)
// aarch64-linux: void @c_arg_u8(i8 %_a)
// arm: void @c_arg_u8(i8 zeroext %_a)
// riscv: void @c_arg_u8(i8 zeroext %_a)
#[no_mangle] pub extern "C" fn c_arg_u8(_a: u8) { }

// x86_64: void @c_arg_u16(i16 zeroext %_a)
// i686: void @c_arg_u16(i16 zeroext %_a)
// aarch64-apple: void @c_arg_u16(i16 zeroext %_a)
// aarch64-windows: void @c_arg_u16(i16 %_a)
// aarch64-linux: void @c_arg_u16(i16 %_a)
// arm: void @c_arg_u16(i16 zeroext %_a)
// riscv: void @c_arg_u16(i16 zeroext %_a)
#[no_mangle] pub extern "C" fn c_arg_u16(_a: u16) { }

// x86_64: void @c_arg_u32(i32 %_a)
// i686: void @c_arg_u32(i32 %_a)
// aarch64-apple: void @c_arg_u32(i32 %_a)
// aarch64-windows: void @c_arg_u32(i32 %_a)
// aarch64-linux: void @c_arg_u32(i32 %_a)
// arm: void @c_arg_u32(i32 %_a)
// riscv: void @c_arg_u32(i32 signext %_a)
#[no_mangle] pub extern "C" fn c_arg_u32(_a: u32) { }

// x86_64: void @c_arg_u64(i64 %_a)
// i686: void @c_arg_u64(i64 %_a)
// aarch64-apple: void @c_arg_u64(i64 %_a)
// aarch64-windows: void @c_arg_u64(i64 %_a)
// aarch64-linux: void @c_arg_u64(i64 %_a)
// arm: void @c_arg_u64(i64 %_a)
// riscv: void @c_arg_u64(i64 %_a)
#[no_mangle] pub extern "C" fn c_arg_u64(_a: u64) { }

// x86_64: void @c_arg_i8(i8 signext %_a)
// i686: void @c_arg_i8(i8 signext %_a)
// aarch64-apple: void @c_arg_i8(i8 signext %_a)
// aarch64-windows: void @c_arg_i8(i8 %_a)
// aarch64-linux: void @c_arg_i8(i8 %_a)
// arm: void @c_arg_i8(i8 signext %_a)
// riscv: void @c_arg_i8(i8 signext %_a)
#[no_mangle] pub extern "C" fn c_arg_i8(_a: i8) { }

// x86_64: void @c_arg_i16(i16 signext %_a)
// i686: void @c_arg_i16(i16 signext %_a)
// aarch64-apple: void @c_arg_i16(i16 signext %_a)
// aarch64-windows: void @c_arg_i16(i16 %_a)
// aarch64-linux: void @c_arg_i16(i16 %_a)
// arm: void @c_arg_i16(i16 signext %_a)
// riscv: void @c_arg_i16(i16 signext %_a)
#[no_mangle] pub extern "C" fn c_arg_i16(_a: i16) { }

// x86_64: void @c_arg_i32(i32 %_a)
// i686: void @c_arg_i32(i32 %_a)
// aarch64-apple: void @c_arg_i32(i32 %_a)
// aarch64-windows: void @c_arg_i32(i32 %_a)
// aarch64-linux: void @c_arg_i32(i32 %_a)
// arm: void @c_arg_i32(i32 %_a)
// riscv: void @c_arg_i32(i32 signext %_a)
#[no_mangle] pub extern "C" fn c_arg_i32(_a: i32) { }

// x86_64: void @c_arg_i64(i64 %_a)
// i686: void @c_arg_i64(i64 %_a)
// aarch64-apple: void @c_arg_i64(i64 %_a)
// aarch64-windows: void @c_arg_i64(i64 %_a)
// aarch64-linux: void @c_arg_i64(i64 %_a)
// arm: void @c_arg_i64(i64 %_a)
// riscv: void @c_arg_i64(i64 %_a)
#[no_mangle] pub extern "C" fn c_arg_i64(_a: i64) { }

// x86_64: zeroext i8 @c_ret_u8()
// i686: zeroext i8 @c_ret_u8()
// aarch64-apple: zeroext i8 @c_ret_u8()
// aarch64-windows: i8 @c_ret_u8()
// aarch64-linux: i8 @c_ret_u8()
// arm: zeroext i8 @c_ret_u8()
// riscv: zeroext i8 @c_ret_u8()
#[no_mangle] pub extern "C" fn c_ret_u8() -> u8 { 0 }

// x86_64: zeroext i16 @c_ret_u16()
// i686: zeroext i16 @c_ret_u16()
// aarch64-apple: zeroext i16 @c_ret_u16()
// aarch64-windows: i16 @c_ret_u16()
// aarch64-linux: i16 @c_ret_u16()
// arm: zeroext i16 @c_ret_u16()
// riscv: zeroext i16 @c_ret_u16()
#[no_mangle] pub extern "C" fn c_ret_u16() -> u16 { 0 }

// x86_64: i32 @c_ret_u32()
// i686: i32 @c_ret_u32()
// aarch64-apple: i32 @c_ret_u32()
// aarch64-windows: i32 @c_ret_u32()
// aarch64-linux: i32 @c_ret_u32()
// arm: i32 @c_ret_u32()
// riscv: signext i32 @c_ret_u32()
#[no_mangle] pub extern "C" fn c_ret_u32() -> u32 { 0 }

// x86_64: i64 @c_ret_u64()
// i686: i64 @c_ret_u64()
// aarch64-apple: i64 @c_ret_u64()
// aarch64-windows: i64 @c_ret_u64()
// aarch64-linux: i64 @c_ret_u64()
// arm: i64 @c_ret_u64()
// riscv: i64 @c_ret_u64()
#[no_mangle] pub extern "C" fn c_ret_u64() -> u64 { 0 }

// x86_64: signext i8 @c_ret_i8()
// i686: signext i8 @c_ret_i8()
// aarch64-apple: signext i8 @c_ret_i8()
// aarch64-windows: i8 @c_ret_i8()
// aarch64-linux: i8 @c_ret_i8()
// arm: signext i8 @c_ret_i8()
// riscv: signext i8 @c_ret_i8()
#[no_mangle] pub extern "C" fn c_ret_i8() -> i8 { 0 }

// x86_64: signext i16 @c_ret_i16()
// i686: signext i16 @c_ret_i16()
// aarch64-apple: signext i16 @c_ret_i16()
// aarch64-windows: i16 @c_ret_i16()
// aarch64-linux: i16 @c_ret_i16()
// arm: signext i16 @c_ret_i16()
// riscv: signext i16 @c_ret_i16()
#[no_mangle] pub extern "C" fn c_ret_i16() -> i16 { 0 }

// x86_64: i32 @c_ret_i32()
// i686: i32 @c_ret_i32()
// aarch64-apple: i32 @c_ret_i32()
// aarch64-windows: i32 @c_ret_i32()
// aarch64-linux: i32 @c_ret_i32()
// arm: i32 @c_ret_i32()
// riscv: signext i32 @c_ret_i32()
#[no_mangle] pub extern "C" fn c_ret_i32() -> i32 { 0 }

// x86_64: i64 @c_ret_i64()
// i686: i64 @c_ret_i64()
// aarch64-apple: i64 @c_ret_i64()
// aarch64-windows: i64 @c_ret_i64()
// aarch64-linux: i64 @c_ret_i64()
// arm: i64 @c_ret_i64()
// riscv: i64 @c_ret_i64()
#[no_mangle] pub extern "C" fn c_ret_i64() -> i64 { 0 }

const C_SOURCE_FILE: &'static str = r##"
#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>

void c_arg_u8(uint8_t _a) { }
void c_arg_u16(uint16_t _a) { }
void c_arg_u32(uint32_t _a) { }
void c_arg_u64(uint64_t _a) { }

void c_arg_i8(int8_t _a) { }
void c_arg_i16(int16_t _a) { }
void c_arg_i32(int32_t _a) { }
void c_arg_i64(int64_t _a) { }

uint8_t c_ret_u8() { return 0; }
uint16_t c_ret_u16() { return 0; }
uint32_t c_ret_u32() { return 0; }
uint64_t c_ret_u64() { return 0; }

int8_t c_ret_i8() { return 0; }
int16_t c_ret_i16() { return 0; }
int32_t c_ret_i32() { return 0; }
int64_t c_ret_i64() { return 0; }
"##;
12 changes: 12 additions & 0 deletions src/test/run-make-fulldeps/issue-97463-abi-param-passing/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
-include ../tools.mk

# The issue exercised by this test, rust-lang/rust#97463, explicitly needs `-O`
# flags (like `-O3`) to reproduce. Thus, we call $(CC) instead of nicer
# alternatives provided by tools.mk like using `COMPILE_OBJ` or using a
# `NATIVE_STATICLIB` dependency.

all:
$(CC) -c -O3 -o $(TMPDIR)/bad.o bad.c
$(AR) rcs $(TMPDIR)/libbad.a $(TMPDIR)/bad.o
$(RUSTC) param_passing.rs -L$(TMPDIR) -lbad -C opt-level=3
$(call RUN,param_passing)
24 changes: 24 additions & 0 deletions src/test/run-make-fulldeps/issue-97463-abi-param-passing/bad.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>


struct bloc {
uint16_t a;
uint16_t b;
uint16_t c;
};

uint16_t c_read_value(uint32_t a, uint32_t b, uint32_t c) {
Copy link
Member

@nagisa nagisa Jun 9, 2022

Choose a reason for hiding this comment

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

We have a test setup for ABI tests with auxiliary/rust_test_helpers.c and ui/abi. Can that mechanism be used instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

let me go inspect that.

(whatever it is, it will need to use -O to replicate the problems here...)

Copy link
Member Author

Choose a reason for hiding this comment

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

auxiliary/rust_test_helpers.c doesn't currently include <stdlib.h>, which is "needed" here if we want to reproduce the original test faithfully, in that it calls malloc. (And given how sensitive reproducing the failure is to small changes, I'm inclined to assume that malloc is a necessity.)

@nagisa do you know offhand if we are deliberately trying to avoid introducing a dependence from rust_test_helpers.c upon stdlib.h and/or malloc itself, perhaps to ensure we can compile the file on esoteric targets that do not have that available? Just curious.

I'll keep moving forward with trying to move this test into auxiliary/rust_test_helpers.c, under the assumption that we can add such a dependence. But given the amount of work I put into getting the codegen tests working, I'm sort of inclined to just live with a run-make test here, if I encounter any hurdles on the way...

struct bloc *data = malloc(sizeof(struct bloc));

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

printf("C struct: a = %u, b = %u, c = %u\n",
(unsigned) data->a, (unsigned) data->b, (unsigned) data->c);
printf("C function returns %u\n", (unsigned) data->b);

return data->b; /* leak data */
}
Loading