Skip to content

Commit

Permalink
Fix passing small structs
Browse files Browse the repository at this point in the history 
The type definitions generated for small struct arguments and returns on
AMD64 and ARM64 wasn't correct, resulting in segmentation faults when
running code with --opt=aggressive. This fixes this issue and ensures we
run tests with all optimization levels on both AMD64 and ARM64.

This fixes #792.
  • Loading branch information
@yorickpeterse
yorickpeterse committed Jan 9, 2025
1 parent 71a9d23 commit cd2e28c
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 45 deletions.
13 changes: 8 additions & 5 deletions .github/workflows/tests.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,12 @@ jobs:
key: amd64-linux-gnu-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml') }}
- name: Run compiler tests
run: cargo test
# We run tests with and without optimizations, such that we can catch any
# potential miscompilations introduced by optimizations. We only do this
# for this particular target as our optimizations aren't target specific.
- name: Run stdlib tests with optimizations
- name: Run stdlib tests with default optimizations
run: 'cd std && cargo run -- test'
- name: Run stdlib tests without optimizations
run: 'cd std && cargo run -- test --opt=none'
- name: Run stdlib tests with aggressive optimizations
run: 'cd std && cargo run -- test --opt=aggressive'

amd64-linux-musl:
timeout-minutes: 15
Expand Down Expand Up @@ -137,8 +136,12 @@ jobs:
run: ./ci/mac.sh
- name: Run compiler tests
run: cargo test
- name: Run stdlib tests
- name: Run stdlib tests with default optimizations
run: 'cd std && cargo run -- test'
- name: Run stdlib tests without optimizations
run: 'cd std && cargo run -- test --opt=none'
- name: Run stdlib tests with aggressive optimizations
run: 'cd std && cargo run -- test --opt=aggressive'

amd64-freebsd-native:
timeout-minutes: 15
Expand Down
131 changes: 102 additions & 29 deletions compiler/src/llvm/context.rs
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use crate::llvm::layouts::{ArgumentType, Layouts, ReturnType};
use crate::state::State;
use crate::target::Architecture;
use crate::target::Target;
use inkwell::attributes::Attribute;
use inkwell::basic_block::BasicBlock;
use inkwell::builder::Builder;
use inkwell::module::Module;
use inkwell::targets::TargetData;
use inkwell::types::{
AnyTypeEnum, ArrayType, BasicType, BasicTypeEnum, FloatType, IntType,
PointerType, StructType, VoidType,
Expand Down Expand Up @@ -110,12 +112,6 @@ impl Context {
self.inner.struct_type(fields, false)
}

pub(crate) fn two_words(&self) -> StructType {
let word = self.i64_type().into();

self.inner.struct_type(&[word, word], false)
}

pub(crate) fn empty_type<'a>(
&'a self,
method_type: StructType<'a>,
Expand Down Expand Up @@ -212,31 +208,26 @@ impl Context {

pub(crate) fn argument_type<'ctx>(
&'ctx self,
state: &State,
layouts: &Layouts<'ctx>,
target_data: &TargetData,
target: Target,
typ: BasicTypeEnum<'ctx>,
) -> ArgumentType<'ctx> {
let BasicTypeEnum::StructType(typ) = typ else {
return ArgumentType::Regular(typ);
};
let bytes = layouts.target_data.get_abi_size(&typ) as u32;
let bytes = target_data.get_abi_size(&typ) as u32;

match state.config.target.arch {
match target.arch {
Architecture::Amd64 => {
if bytes <= 8 {
let bits = self.custom_int(size_in_bits(bytes));

ArgumentType::Regular(bits.as_basic_type_enum())
} else if bytes <= 16 {
// The AMD64 ABI doesn't handle types such as
// `{ i16, i64 }`. While it does handle `{ i64, i16 }`, this
// requires re-ordering the fields and their corresponding
// access sites.
//
// To avoid the complexity of that we take the same approach
// as Rust: if the struct is larger than 8 bytes, we turn it
// into two 64 bits values.
ArgumentType::Regular(self.two_words().as_basic_type_enum())
ArgumentType::Regular(
self.small_amd64_struct(target_data, typ)
.as_basic_type_enum(),
)
} else {
ArgumentType::StructValue(typ)
}
Expand All @@ -245,7 +236,11 @@ impl Context {
if bytes <= 8 {
ArgumentType::Regular(self.i64_type().as_basic_type_enum())
} else if bytes <= 16 {
ArgumentType::Regular(self.two_words().as_basic_type_enum())
// TODO: handle 4x float values
ArgumentType::Regular(
self.small_arm64_struct(target_data, typ)
.as_basic_type_enum(),
)
} else {
// clang and Rust don't use "byval" for ARM64 when the
// struct is too large, so neither do we.
Expand All @@ -268,45 +263,105 @@ impl Context {
method.return_type(&state.db),
);

self.return_type(state, layouts, typ)
self.return_type(layouts.target_data, state.config.target, typ)
} else {
ReturnType::None
}
}

pub(crate) fn return_type<'ctx>(
&'ctx self,
state: &State,
layouts: &Layouts<'ctx>,
target_data: &TargetData,
target: Target,
typ: BasicTypeEnum<'ctx>,
) -> ReturnType<'ctx> {
let BasicTypeEnum::StructType(typ) = typ else {
return ReturnType::Regular(typ);
};

let bytes = layouts.target_data.get_abi_size(&typ) as u32;
let bytes = target_data.get_abi_size(&typ) as u32;

match state.config.target.arch {
// For both AMD64 and ARM64 the way structs are returned is the
// same. For more details, refer to argument_type().
Architecture::Amd64 | Architecture::Arm64 => {
match target.arch {
Architecture::Amd64 => {
if bytes <= 8 {
let bits = self.custom_int(size_in_bits(bytes));

ReturnType::Regular(bits.as_basic_type_enum())
} else if bytes <= 16 {
ReturnType::Regular(self.two_words().as_basic_type_enum())
ReturnType::Regular(
self.small_amd64_struct(target_data, typ)
.as_basic_type_enum(),
)
} else {
ReturnType::Struct(typ)
}
}
Architecture::Arm64 => {
if bytes <= 8 {
ReturnType::Regular(self.i64_type().as_basic_type_enum())
} else if bytes <= 16 {
// TODO: handle 4x float values
ReturnType::Regular(
self.small_arm64_struct(target_data, typ)
.as_basic_type_enum(),
)
} else {
ReturnType::Struct(typ)
}
}
}
}

fn small_arm64_struct<'ctx>(
&'ctx self,
target_data: &TargetData,
typ: StructType<'ctx>,
) -> StructType<'ctx> {
let size = target_data.get_abi_size(&typ) as u32;
let align = target_data.get_abi_alignment(&typ);
let rest = max(size - 8, align);
let a = self.i64_type().into();
let b = self.custom_int(size_in_bits(rest)).into();

self.struct_type(&[a, b])
}

fn small_amd64_struct<'ctx>(
&'ctx self,
target_data: &TargetData,
typ: StructType<'ctx>,
) -> StructType<'ctx> {
let size = target_data.get_abi_size(&typ) as u32;
let align = target_data.get_abi_alignment(&typ);
let rest = max(size - 8, align);
let a = self.i64_type().into();
let b = self.custom_int(size_in_bits(rest)).into();

self.struct_type(&[a, b])
}
}

#[cfg(test)]
mod tests {
use super::*;
use crate::target::{Abi, Architecture, OperatingSystem};
use inkwell::targets::{
CodeModel, InitializationConfig, RelocMode, Target as LlvmTarget,
TargetMachine, TargetTriple,
};
use inkwell::OptimizationLevel;

fn setup(target: Target) -> TargetMachine {
let triple = TargetTriple::create(&target.llvm_triple());
let reloc = RelocMode::PIC;
let model = CodeModel::Default;
let level = OptimizationLevel::None;

LlvmTarget::from_triple(&triple)
.unwrap()
.create_target_machine(&triple, "", "", level, reloc, model)
.unwrap()
}

#[test]
fn test_context_type_sizes() {
Expand All @@ -318,4 +373,22 @@ mod tests {
assert_eq!(ctx.rust_string_type().len(), 24);
assert_eq!(ctx.rust_vec_type().len(), 24);
}

#[test]
fn test_argument_type_for_amd64_with_less_than_one_word() {
LlvmTarget::initialize_x86(&InitializationConfig::default());

let target =
Target::new(Architecture::Amd64, OperatingSystem::Linux, Abi::Gnu);
let machine = setup(target);
let ctx = Context::new();
let inp = ctx.struct_type(&[ctx.i8_type().into()]).as_basic_type_enum();
let ArgumentType::Regular(BasicTypeEnum::IntType(out)) =
ctx.argument_type(&machine.get_target_data(), target, inp)
else {
panic!("unexpected type")
};

assert_eq!(out.get_bit_width(), 8);
}
}
6 changes: 5 additions & 1 deletion compiler/src/llvm/layouts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,11 @@ impl<'ctx> Method<'ctx> {
.chain(method.argument_types(db))
{
let raw = context.llvm_type(db, layouts, typ);
let typ = context.argument_type(state, layouts, raw);
let typ = context.argument_type(
layouts.target_data,
state.config.target,
raw,
);

args.push(typ);
}
Expand Down
12 changes: 6 additions & 6 deletions compiler/src/llvm/passes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2062,8 +2062,8 @@ impl<'shared, 'module, 'ctx> LowerMethod<'shared, 'module, 'ctx> {
let mut layout = MethodLayout::new();

layout.returns = self.builder.context.return_type(
self.shared.state,
self.layouts,
self.layouts.target_data,
self.shared.state.config.target,
reg_typ,
);

Expand All @@ -2074,8 +2074,8 @@ impl<'shared, 'module, 'ctx> LowerMethod<'shared, 'module, 'ctx> {
for reg in [ins.receiver].iter().chain(ins.arguments.iter()) {
let raw = self.variable_types[reg];
let typ = self.builder.context.argument_type(
self.shared.state,
self.layouts,
self.layouts.target_data,
self.shared.state.config.target,
raw,
);

Expand Down Expand Up @@ -2130,8 +2130,8 @@ impl<'shared, 'module, 'ctx> LowerMethod<'shared, 'module, 'ctx> {

layout.returns = ReturnType::Regular(reg_typ);
layout.arguments.push(self.builder.context.argument_type(
self.shared.state,
self.layouts,
self.layouts.target_data,
self.shared.state.config.target,
rec_typ,
));

Expand Down
8 changes: 4 additions & 4 deletions compiler/src/target.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::fmt;
pub(crate) const MAC_SDK_VERSION: &str = "11.0.0";

/// The supported CPU architectures.
#[derive(Eq, PartialEq, Debug)]
#[derive(Eq, PartialEq, Debug, Copy, Clone)]
pub(crate) enum Architecture {
Amd64,
Arm64,
Expand All @@ -34,7 +34,7 @@ impl Architecture {
}

/// The supported operating systems.
#[derive(Eq, PartialEq, Debug)]
#[derive(Eq, PartialEq, Debug, Copy, Clone)]
pub(crate) enum OperatingSystem {
Freebsd,
Linux,
Expand Down Expand Up @@ -73,7 +73,7 @@ impl OperatingSystem {
}

/// The ABI to target.
#[derive(Eq, PartialEq, Debug)]
#[derive(Eq, PartialEq, Debug, Copy, Clone)]
pub(crate) enum Abi {
Native,
Gnu,
Expand Down Expand Up @@ -107,7 +107,7 @@ impl Abi {

/// A type describing the compile target, such as the operating system and
/// architecture.
#[derive(Eq, PartialEq, Debug)]
#[derive(Eq, PartialEq, Debug, Copy, Clone)]
pub struct Target {
pub(crate) arch: Architecture,
pub(crate) os: OperatingSystem,
Expand Down

0 comments on commit cd2e28c

Please sign in to comment.