Skip to content
This repository was archived by the owner on Jun 10, 2024. It is now read-only.

Commit ecbffc5

Browse files
bitwalkerbitwalker
committed
fix: a few abi/smart pointer bugs
- A previous change introduced a regression in the GcBox/Rc implementation that caused SIGSEGV errors - We were not properly guaranteeing the alignment of certain unsized types - Some mistyped getelementptr/store instructions in the make_fun impl
1 parent 583c836 commit ecbffc5

File tree

17 files changed

+129
-54
lines changed

17 files changed

+129
-54
lines changed

‎compiler/mlir/c_src/lib/CIR/ConvertCIRToLLVMPass.cpp

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -274,8 +274,8 @@ class CIRTypeConverter : public LLVMTypeConverter {
274274
if (closureTy.isInitialized())
275275
return closureTy;
276276

277-
auto atomTy = getTermType();
278-
auto arityTy = IntegerType::get(context, 8);
277+
auto atomTy = LLVM::LLVMPointerType::get(getAtomDataType());
278+
auto arityTy = getIsizeType();
279279
auto bareFunTy = LLVM::LLVMFunctionType::get(
280280
LLVM::LLVMVoidType::get(context), {}, /*vararg=*/false);
281281
auto funPtrTy = LLVM::LLVMPointerType::get(bareFunTy);
@@ -843,6 +843,23 @@ class CIRConversionPattern : public ConversionPattern {
843843
return encodeAtomPtr(builder, loc, ptr);
844844
}
845845

846+
// Used like `createAtom`, but for situations in which we are not encoding as
847+
// a term
848+
Value createAtomData(OpBuilder &builder, Location loc, StringRef name,
849+
ModuleOp &module) const {
850+
if (name == "false") {
851+
auto dataTy = LLVM::LLVMPointerType::get(getAtomDataType());
852+
Value zero = createIsizeConstant(builder, loc, 0);
853+
return builder.create<LLVM::IntToPtrOp>(loc, dataTy, zero);
854+
} else if (name == "true") {
855+
auto dataTy = LLVM::LLVMPointerType::get(getAtomDataType());
856+
Value one = createIsizeConstant(builder, loc, 1);
857+
return builder.create<LLVM::IntToPtrOp>(loc, dataTy, one);
858+
}
859+
860+
return createAtomDataGlobal(builder, loc, module, name);
861+
}
862+
846863
// This function constructs an AtomData record as a global constant,
847864
// referencing the atom data as a separate global containing a null-terminated
848865
// string. Returns the address to the data record
@@ -2295,8 +2312,10 @@ struct MakeFunOpLowering : public ConvertCIROpToLLVMPattern<cir::MakeFunOp> {
22952312
auto closureTy = getClosureType();
22962313
auto closurePtrTy = LLVM::LLVMPointerType::get(closureTy);
22972314
auto termPtrTy = LLVM::LLVMPointerType::get(termTy);
2298-
auto i8Ty = getI8Type();
2299-
auto i8PtrTy = LLVM::LLVMPointerType::get(i8Ty);
2315+
auto atomDataPtrTy = LLVM::LLVMPointerType::get(getAtomDataType());
2316+
auto atomDataPtrPtrTy = LLVM::LLVMPointerType::get(atomDataPtrTy);
2317+
auto isizeTy = getIsizeType();
2318+
auto isizePtrTy = LLVM::LLVMPointerType::get(isizeTy);
23002319

23012320
Value mallocPtr = rewriter.create<cir::MallocOp>(loc, TypeAttr::get(funTy));
23022321
Operation *ptrCast = rewriter.create<UnrealizedConversionCastOp>(
@@ -2331,33 +2350,34 @@ struct MakeFunOpLowering : public ConvertCIROpToLLVMPattern<cir::MakeFunOp> {
23312350
}
23322351

23332352
// Store the module atom
2334-
auto moduleAtom = createAtom(rewriter, loc, moduleName, module);
2353+
auto moduleAtom = createAtomData(rewriter, loc, moduleName, module);
23352354
Value zero = createI32Constant(rewriter, loc, 0);
2336-
auto modulePtr = rewriter.create<LLVM::GEPOp>(loc, termPtrTy, ptr,
2355+
auto modulePtr = rewriter.create<LLVM::GEPOp>(loc, atomDataPtrPtrTy, ptr,
23372356
ValueRange({zero, zero}));
23382357
rewriter.create<LLVM::StoreOp>(loc, moduleAtom, modulePtr);
23392358

23402359
// Store the function atom
2341-
auto functionAtom = createAtom(rewriter, loc, functionName, module);
2360+
auto functionAtom = createAtomData(rewriter, loc, functionName, module);
23422361
Value one = createI32Constant(rewriter, loc, 1);
2343-
auto functionPtr = rewriter.create<LLVM::GEPOp>(loc, termPtrTy, ptr,
2362+
auto functionPtr = rewriter.create<LLVM::GEPOp>(loc, atomDataPtrPtrTy, ptr,
23442363
ValueRange({zero, one}));
23452364
rewriter.create<LLVM::StoreOp>(loc, functionAtom, functionPtr);
23462365

2347-
// Store the arity (i8)
2348-
auto arityInt = createI8Constant(rewriter, loc, arity);
2366+
// Store the arity (isize)
2367+
auto arityInt = createIsizeConstant(rewriter, loc, arity);
23492368
Value two = createI32Constant(rewriter, loc, 2);
2350-
auto arityPtr = rewriter.create<LLVM::GEPOp>(loc, i8PtrTy, ptr,
2369+
auto arityPtr = rewriter.create<LLVM::GEPOp>(loc, isizePtrTy, ptr,
23512370
ValueRange({zero, two}));
23522371
rewriter.create<LLVM::StoreOp>(loc, arityInt, arityPtr);
23532372

23542373
// Store the callee pointer
2355-
Value three = createI32Constant(rewriter, loc, 3);
2356-
auto calleePtr = rewriter.create<LLVM::GEPOp>(loc, i8PtrTy, ptr,
2357-
ValueRange({zero, three}));
23582374
auto opaqueFunTy =
23592375
LLVM::LLVMFunctionType::get(getVoidType(), ArrayRef<Type>{});
23602376
auto opaqueFunPtrTy = LLVM::LLVMPointerType::get(opaqueFunTy);
2377+
auto opaqueFunPtrPtrTy = LLVM::LLVMPointerType::get(opaqueFunPtrTy);
2378+
Value three = createI32Constant(rewriter, loc, 3);
2379+
auto calleePtr = rewriter.create<LLVM::GEPOp>(loc, opaqueFunPtrPtrTy, ptr,
2380+
ValueRange({zero, three}));
23612381
auto calleeRaw =
23622382
rewriter.create<LLVM::BitcastOp>(loc, opaqueFunPtrTy, callee);
23632383
rewriter.create<LLVM::StoreOp>(loc, calleeRaw, calleePtr);
@@ -2390,6 +2410,7 @@ struct MakeFunOpLowering : public ConvertCIROpToLLVMPattern<cir::MakeFunOp> {
23902410
auto envPtr = rewriter.create<LLVM::GEPOp>(
23912411
loc, termPtrTy, ptr, ValueRange({zero, four, envIdxConst}));
23922412
rewriter.create<LLVM::StoreOp>(loc, env, envPtr);
2413+
envIdx++;
23932414
}
23942415

23952416
// Box the pointer to the allocation

‎library/alloc/Cargo.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ authors = ["Paul Schoenfelder <[email protected]>"]
66
publish = false
77
edition = "2021"
88

9+
[features]
10+
default = []
11+
std = ["firefly_binary/std"]
12+
913
[dependencies]
1014
firefly_binary = { path = "../binary" }
1115
firefly_system = { path = "../system" }

‎library/alloc/src/gc/boxed/gcbox.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ where
5858
let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
5959
let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
6060
unsafe {
61-
let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
61+
let ptr = NonNull::new_unchecked(ptr.as_ptr().byte_add(value_offset));
6262
let boxed = Self {
6363
ptr,
6464
_marker: PhantomData,
@@ -74,7 +74,7 @@ where
7474
let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
7575
let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
7676
unsafe {
77-
let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
77+
let ptr = NonNull::new_unchecked(ptr.as_ptr().byte_add(value_offset));
7878
let meta = Metadata::new::<T>(ptr.as_ptr() as *mut T);
7979
let boxed = GcBox {
8080
ptr,
@@ -105,7 +105,7 @@ where
105105
let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
106106
let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
107107
unsafe {
108-
let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
108+
let ptr = NonNull::new_unchecked(ptr.as_ptr().byte_add(value_offset));
109109
let boxed = Self {
110110
ptr,
111111
_marker: PhantomData,
@@ -241,7 +241,7 @@ where
241241
if layout.size() > 0 {
242242
ptr::drop_in_place(value);
243243
}
244-
let ptr = NonNull::new_unchecked(boxed.ptr.as_ptr().sub(value_offset));
244+
let ptr = NonNull::new_unchecked(boxed.ptr.as_ptr().byte_sub(value_offset));
245245
alloc.deallocate(ptr.cast(), layout);
246246
}
247247

@@ -351,7 +351,7 @@ where
351351
let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
352352
let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
353353
unsafe {
354-
let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
354+
let ptr = NonNull::new_unchecked(ptr.as_ptr().byte_add(value_offset));
355355
let boxed = Self {
356356
ptr,
357357
_marker: PhantomData,

‎library/alloc/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
#![feature(const_type_id)]
2626

2727
extern crate alloc;
28+
#[cfg(feature = "std")]
29+
extern crate std;
2830
#[cfg(test)]
2931
extern crate test;
3032

‎library/alloc/src/rc/mod.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ where
239239
let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
240240
let ptr: NonNull<()> = Global.allocate(layout).unwrap().cast();
241241
unsafe {
242-
let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
242+
let ptr = NonNull::new_unchecked(ptr.as_ptr().byte_add(value_offset));
243243
let boxed = Self {
244244
ptr,
245245
_marker: PhantomData,
@@ -255,7 +255,7 @@ where
255255
let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
256256
let ptr: NonNull<()> = Global.allocate(layout).unwrap().cast();
257257
unsafe {
258-
let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
258+
let ptr = NonNull::new_unchecked(ptr.as_ptr().byte_add(value_offset));
259259
let meta = Metadata::new::<T>(ptr.as_ptr() as *mut T);
260260
let boxed = Rc {
261261
ptr,
@@ -279,7 +279,7 @@ where
279279
let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
280280
let ptr: NonNull<()> = Global.allocate(layout).unwrap().cast();
281281
unsafe {
282-
let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
282+
let ptr = NonNull::new_unchecked(ptr.as_ptr().byte_add(value_offset));
283283
let boxed = Self {
284284
ptr,
285285
_marker: PhantomData,
@@ -431,7 +431,7 @@ where
431431
if layout.size() > 0 {
432432
ptr::drop_in_place(value);
433433
}
434-
let ptr = NonNull::new_unchecked(self.ptr.as_ptr().sub(value_offset));
434+
let ptr = NonNull::new_unchecked(self.ptr.as_ptr().byte_sub(value_offset));
435435
Global.deallocate(ptr.cast(), layout);
436436
}
437437
}
@@ -594,7 +594,7 @@ where
594594
let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
595595
let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
596596
unsafe {
597-
let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
597+
let ptr = NonNull::new_unchecked(ptr.as_ptr().byte_add(value_offset));
598598
let boxed = Self {
599599
ptr,
600600
_marker: PhantomData,

‎library/rt/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ edition = "2021"
88

99
[features]
1010
default = ["std"]
11-
std = ["anyhow/std", "backtrace/std", "num-bigint/std", "rpds/std", "termcolor", "firefly_binary/std"]
11+
std = ["anyhow/std", "backtrace/std", "num-bigint/std", "rpds/std", "termcolor", "firefly_binary/std", "firefly_alloc/std"]
1212
no_std = ["lazy_static/spin_no_std"]
1313

1414
[dependencies]

‎library/rt/src/term/binary/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ use firefly_binary::{Aligned, Binary, BinaryFlags, Bitstring, Encoding};
1919

2020
/// This represents binary data, i.e. byte-aligned, with a number of bits
2121
/// divisible by 8 evenly.
22-
#[repr(C)]
22+
#[repr(C, align(16))]
2323
pub struct BinaryData {
2424
flags: BinaryFlags,
2525
data: [u8],

‎library/rt/src/term/closure.rs

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,20 +22,20 @@ use super::{Atom, OpaqueTerm};
2222
/// the callee to access the closed-over values from its environment.
2323
///
2424
/// Function captures do not have the extra self argument, and always have an implicitly empty environment.
25-
#[repr(C)]
25+
#[repr(C, align(16))]
2626
pub struct Closure {
2727
pub module: Atom,
2828
pub name: Atom,
29-
pub arity: u8,
29+
pub arity: usize,
3030
fun: *const (),
3131
env: [OpaqueTerm],
3232
}
3333
impl fmt::Debug for Closure {
3434
#[inline]
3535
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
3636
f.debug_struct("Closure")
37-
.field("module", &self.module)
38-
.field("function", &self.name)
37+
.field("module", &self.module.as_str())
38+
.field("function", &self.name.as_str())
3939
.field("arity", &self.arity)
4040
.field("fun", &self.fun)
4141
.field("env", &&self.env)
@@ -70,7 +70,7 @@ impl Closure {
7070
let mut this = GcBox::<Self>::with_capacity_in(env.len(), alloc)?;
7171
this.module = module;
7272
this.name = name;
73-
this.arity = arity;
73+
this.arity = arity as usize;
7474
this.fun = fun;
7575
this.env.copy_from_slice(env);
7676
Ok(this)
@@ -137,10 +137,10 @@ seq!(A in 0..10 {
137137
/// This type represents a function which implements a closure of arity A
138138
///
139139
/// See the `Closure` docs for more information on how closures are implemented.
140-
pub type Closure~A = extern "C" fn (&Closure,
141-
#(
140+
pub type Closure~A = extern "C" fn (#(
142141
OpaqueTerm,
143142
)*
143+
OpaqueTerm
144144
) -> ErlangResult;
145145

146146
/// This type represents a function capture of arity A
@@ -163,7 +163,8 @@ seq!(A in 0..10 {
163163
} else {
164164
assert_eq!(self.arity, A + 1, "mismatched arity");
165165
let fun = unsafe { core::mem::transmute::<_, Closure~A>(self.fun) };
166-
fun(self, #(_args.N,)*)
166+
let this = unsafe { OpaqueTerm::from_gcbox_closure(self) };
167+
fun(#(_args.N,)* this)
167168
}
168169
}
169170
}
@@ -177,7 +178,8 @@ seq!(A in 0..10 {
177178
} else {
178179
assert_eq!(self.arity, A + 1, "mismatched arity");
179180
let fun = unsafe { core::mem::transmute::<_, Closure~A>(self.fun) };
180-
fun(self, #(_args.N,)*)
181+
let this = unsafe { OpaqueTerm::from_gcbox_closure(self) };
182+
fun(#(_args.N,)* this)
181183
}
182184
}
183185
}
@@ -191,7 +193,8 @@ seq!(A in 0..10 {
191193
} else {
192194
assert_eq!(self.arity, A + 1, "mismatched arity");
193195
let fun = unsafe { core::mem::transmute::<_, Closure~A>(self.fun) };
194-
fun(self, #(_args.N,)*)
196+
let this = unsafe { OpaqueTerm::from_gcbox_closure(self) };
197+
fun(#(_args.N,)* this)
195198
}
196199
}
197200
}

‎library/rt/src/term/list.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,9 @@ impl Cons {
4646
/// NOTE: The returned cell is wrapped in `MaybeUninit<T>` because the head/tail require
4747
/// initialization.
4848
pub fn new_in<A: Allocator>(alloc: A) -> Result<NonNull<Cons>, AllocError> {
49-
alloc.allocate(Layout::new::<Cons>()).map(|ptr| ptr.cast())
49+
alloc
50+
.allocate_zeroed(Layout::new::<Cons>())
51+
.map(|ptr| ptr.cast())
5052
}
5153

5254
/// Constructs a list from the given slice, the output of which will be in the same order as the slice.

0 commit comments

Comments
 (0)