fix: invalid alignment of tuple pointers

The issue here presented as tuples printing off the end of their
elements array by some large arbitrary amount. This was happening
because the memory we allocated for a tuple included space for the
pointer metadata (the size) we need to restore fat pointers for tuples
in Rust, so we would offset the base pointer of the allocation by
`mem::size_of::<usize>()` bytes and return _that_ as the pointer to the
tuple, and in cases where we were decoding the tuple, we'd subtract the
offset to read the metadata and then use the metadata to restore a fat
pointer.

The problem here, is that by offsetting the pointer we allocated, we
were changing the alignment of the pointer, and the new alignment was
not 16 bytes, but 8, and therefore when encoding as an OpaqueTerm, the
low 4 bits of the pointer were being masked off, effectively encoding a
pointer to the base of the allocation, rather than to the start of the
tuple elements. Then, when any mutation of a tuple involving the first
element of the tuple occurred, it would overwrite the capacity metadata
with an OpaqueTerm value, then any subsequent traversal of the tuple
elements would think it was walking a tuple of like 9 billion elements.

The solution here is simple, we have no good reason to offset the
pointer we're encoding as a term, so instead we change the definition of
Tuple to make it clear that the base of the struct includes the capacity
metadata, and update all code that cares about the layout of the Tuple
struct to be aware of this new layout. The pointers are properly
aligned, and we don't have to worry about accidentally overwriting the
metadata any longer.

NOTE: This problem isn't present with Closure or BinaryData, which are
also unsized types, because they already made their metadata field
explicit in the struct definition.

This fixes the issue with the global_dynamic_call test.

Author: bitwalker

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

@@ -201,8 +201,11 @@ class CIRTypeConverter : public LLVMTypeConverter {
   // This layout matches what our runtime produces/expects, and we rely on this
   // to optimize operations on them
   Type getTupleType(unsigned arity) {
+    MLIRContext *context = &getContext();
+    auto isizeTy = getIsizeType();
     auto termTy = getTermType();
-    return LLVM::LLVMArrayType::get(termTy, arity);
+    auto elemsTy = LLVM::LLVMArrayType::get(termTy, arity);
+    return LLVM::LLVMStructType::getLiteral(context, {isizeTy, elemsTy});
   }
 
   // This layout is intentionally incomplete, as we don't control the layout of
@@ -293,8 +296,8 @@ class CIRTypeConverter : public LLVMTypeConverter {
     if (gcBoxTy.isInitialized())
       return gcBoxTy;
 
-    auto metadataTy = LLVM::LLVMStructType::getIdentified(
-        context, "firefly_alloc::Metadata");
+    auto metadataTy =
+        LLVM::LLVMStructType::getIdentified(context, "firefly_alloc::Metadata");
     auto typeIdTy = getIsizeType();
     auto ptrMetadataTy = getIsizeType();
     assert(succeeded(metadataTy.setBody({typeIdTy, ptrMetadataTy},
@@ -762,9 +765,9 @@ class CIRConversionPattern : public ConversionPattern {
                                 "__firefly_bigint_from_digits", calleeType);
     }
 
-    Operation *call = builder.create<LLVM::CallOp>(loc, TypeRange({termTy}),
-                                                   "__firefly_bigint_from_digits",
-                                                   ValueRange({fatPtr}));
+    Operation *call = builder.create<LLVM::CallOp>(
+        loc, TypeRange({termTy}), "__firefly_bigint_from_digits",
+        ValueRange({fatPtr}));
     return call->getResult(0);
   }
 
@@ -1744,8 +1747,8 @@ struct TypeOfOpLowering : public ConvertCIROpToLLVMPattern<cir::TypeOfOp> {
     if (!callee) {
       auto calleeType =
           LLVM::LLVMFunctionType::get(i32Ty, ArrayRef<Type>{termTy});
-      insertFunctionDeclaration(rewriter, loc, module, "__firefly_builtin_typeof",
-                                calleeType);
+      insertFunctionDeclaration(rewriter, loc, module,
+                                "__firefly_builtin_typeof", calleeType);
     }
 
     rewriter.replaceOpWithNewOp<LLVM::CallOp>(op, TypeRange({i32Ty}),
@@ -1913,11 +1916,12 @@ struct IsTaggedTupleOpLowering
           Value tuplePtr = decodePtr(builder, l, tupleTy, input);
           SmallVector<Value> indices;
           // This first index refers to the base of the tuple struct
-          // This second index refers to the first element of the tuple data
+          // This second index refers to the base of the elements array
+          // This third index refers to the first element of the array
           Value zero = createIsizeConstant(builder, l, 0);
           Value first = createIndexAttrConstant(builder, l, i32Ty, 0);
           Value elemPtr = builder.create<LLVM::GEPOp>(
-              l, termPtrTy, tuplePtr, ValueRange({zero, first}));
+              l, termPtrTy, tuplePtr, ValueRange({zero, first, first}));
           Value elem = builder.create<LLVM::LoadOp>(l, elemPtr);
           Value expectedAtom = createAtom(builder, l, atom.getName(), module);
           Value isEq = builder.create<LLVM::ICmpOp>(l, LLVM::ICmpPredicate::eq,
@@ -2185,8 +2189,8 @@ struct MallocOpLowering : public ConvertCIROpToLLVMPattern<cir::MallocOp> {
     if (!callee) {
       auto calleeType =
           LLVM::LLVMFunctionType::get(i8PtrTy, ArrayRef<Type>{i32Ty, isizeTy});
-      insertFunctionDeclaration(rewriter, loc, module, "__firefly_builtin_malloc",
-                                calleeType);
+      insertFunctionDeclaration(rewriter, loc, module,
+                                "__firefly_builtin_malloc", calleeType);
     }
 
     Type outType;
@@ -2521,10 +2525,11 @@ struct GetElementOpLowering
     Value ptr = decodePtr(rewriter, loc, tupleTy, tuple);
     // Then, calculate the pointer to the <index>th element
     Value base = createI32Constant(rewriter, loc, 0);
+    Value one = createI32Constant(rewriter, loc, 1);
     Value index =
         createI32Constant(rewriter, loc, adaptor.index().getLimitedValue());
-    Value elemPtr = rewriter.create<LLVM::GEPOp>(loc, termPtrTy, ptr,
-                                                 ValueRange({base, index}));
+    Value elemPtr = rewriter.create<LLVM::GEPOp>(
+        loc, termPtrTy, ptr, ValueRange({base, one, index}));
     // Then return the result of loading the value from the calculated pointer
     rewriter.replaceOpWithNewOp<LLVM::LoadOp>(op, elemPtr);
     return success();
@@ -2567,8 +2572,8 @@ struct SetElementOpLowering
         auto tuplePtrTy = LLVM::LLVMPointerType::get(tupleTy);
         auto calleeType = LLVM::LLVMFunctionType::get(
             termTy, ArrayRef<Type>{tuplePtrTy, isizeTy, termTy});
-        insertFunctionDeclaration(rewriter, loc, module, "__firefly_set_element",
-                                  calleeType);
+        insertFunctionDeclaration(rewriter, loc, module,
+                                  "__firefly_set_element", calleeType);
       }
       rewriter.replaceOpWithNewOp<LLVM::CallOp>(
           op, TypeRange({termTy}), "__firefly_set_element",
@@ -2579,10 +2584,11 @@ struct SetElementOpLowering
     // If we reach here, we are allowed to mutate the original tuple in-place;
     // Then, calculate the pointer to the <index>th element
     Value base = createI32Constant(rewriter, loc, 0);
+    Value one = createI32Constant(rewriter, loc, 1);
     Value index32 =
         createI32Constant(rewriter, loc, adaptor.index().getLimitedValue());
-    Value elemPtr = rewriter.create<LLVM::GEPOp>(loc, tuplePtrTy, ptr,
-                                                 ValueRange({base, index32}));
+    Value elemPtr = rewriter.create<LLVM::GEPOp>(
+        loc, tuplePtrTy, ptr, ValueRange({base, one, index32}));
     // Then store the input value at the calculated pointer
     rewriter.create<LLVM::StoreOp>(loc, value, elemPtr);
     rewriter.replaceOp(op, ValueRange({tuple}));
@@ -2967,8 +2973,8 @@ struct BinaryMatchStartOpLowering
     if (!callee) {
       auto calleeType =
           LLVM::LLVMFunctionType::get(resultTy, ArrayRef<Type>{termTy});
-      insertFunctionDeclaration(rewriter, loc, module, "__firefly_bs_match_start",
-                                calleeType);
+      insertFunctionDeclaration(rewriter, loc, module,
+                                "__firefly_bs_match_start", calleeType);
     }
     auto callOp = rewriter.create<LLVM::CallOp>(loc, TypeRange({resultTy}),
                                                 "__firefly_bs_match_start",

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

@@ -16,7 +16,7 @@ where
     T: ?Sized + 'static,
     PtrMetadata: From<<T as Pointee>::Metadata> + TryInto<<T as Pointee>::Metadata>,
 {
-    ptr: NonNull<u8>,
+    ptr: NonNull<()>,
     _marker: PhantomData<T>,
 }
 
@@ -56,7 +56,7 @@ where
         let meta = Metadata::new::<T>(&value);
         let value_layout = Layout::for_value(&value);
         let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
-        let ptr: NonNull<u8> = alloc.allocate(layout)?.cast();
+        let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
         unsafe {
             let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
             let boxed = Self {
@@ -72,7 +72,7 @@ where
     pub fn new_uninit_in<A: Allocator>(alloc: A) -> Result<GcBox<MaybeUninit<T>>, AllocError> {
         let value_layout = Layout::new::<T>();
         let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
-        let ptr: NonNull<u8> = alloc.allocate(layout)?.cast();
+        let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
         unsafe {
             let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
             let meta = Metadata::new::<T>(ptr.as_ptr() as *mut T);
@@ -103,7 +103,7 @@ where
         let meta = Metadata::new::<T>(unsized_);
         let value_layout = Layout::for_value(&value);
         let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
-        let ptr: NonNull<u8> = alloc.allocate(layout)?.cast();
+        let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
         unsafe {
             let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
             let boxed = Self {
@@ -136,7 +136,7 @@ where
         };
         // Construct a new box
         let boxed = GcBox {
-            ptr: unsafe { NonNull::new_unchecked(ptr as *mut u8) },
+            ptr: unsafe { NonNull::new_unchecked(ptr as *mut ()) },
             _marker: PhantomData,
         };
         // Write the new metadata to the header for our new box
@@ -157,7 +157,7 @@ where
     /// use a combination of this function, a jump table of type ids, and subsequent unchecked casts.
     /// This allows for efficient pointer casts while ensuring we don't improperly cast a pointer to
     /// the wrong type.
-    pub unsafe fn type_id(raw: *mut u8) -> TypeId {
+    pub unsafe fn type_id(raw: *mut ()) -> TypeId {
         debug_assert!(!raw.is_null());
         let header = &*header(raw);
         header.ty
@@ -171,7 +171,7 @@ where
     ///
     /// NOTE: This function is a bit safer than `from_raw` in that it won't panic if the pointee
     /// is not of the correct type, instead it returns `Err`.
-    pub unsafe fn try_from_raw(raw: *mut u8) -> Result<Self, ()> {
+    pub unsafe fn try_from_raw(raw: *mut ()) -> Result<Self, ()> {
         debug_assert!(!raw.is_null());
         let meta = &*header(raw);
         if meta.is::<T>() {
@@ -215,7 +215,7 @@ where
     /// but ensure that the pointee is the correct type.
     ///
     /// NOTE: Seriously, don't use this.
-    pub unsafe fn from_raw_unchecked(raw: *mut u8) -> Self {
+    pub unsafe fn from_raw_unchecked(raw: *mut ()) -> Self {
         debug_assert!(!raw.is_null());
         Self {
             ptr: NonNull::new_unchecked(raw),
@@ -229,7 +229,7 @@ where
     }
 
     #[inline]
-    pub fn as_ptr(boxed: &Self) -> *mut u8 {
+    pub fn as_ptr(boxed: &Self) -> *mut () {
         boxed.ptr.as_ptr()
     }
 
@@ -242,7 +242,7 @@ where
             ptr::drop_in_place(value);
         }
         let ptr = NonNull::new_unchecked(boxed.ptr.as_ptr().sub(value_offset));
-        alloc.deallocate(ptr, layout);
+        alloc.deallocate(ptr.cast(), layout);
     }
 
     #[inline]
@@ -261,7 +261,7 @@ impl<T: ?Sized + 'static + Pointee<Metadata = usize>> GcBox<T> {
     {
         let raw = Self::into_raw(self) as *mut U;
         GcBox {
-            ptr: unsafe { NonNull::new_unchecked(raw as *mut u8) },
+            ptr: unsafe { NonNull::new_unchecked(raw as *mut ()) },
             _marker: PhantomData,
         }
     }
@@ -279,7 +279,7 @@ impl GcBox<dyn Any> {
         let Some(concrete) = any.downcast_ref::<T>() else { return None; };
         let meta = Metadata::new::<T>(concrete);
         let boxed = GcBox {
-            ptr: unsafe { NonNull::new_unchecked(concrete as *const _ as *mut u8) },
+            ptr: unsafe { NonNull::new_unchecked(concrete as *const _ as *mut ()) },
             _marker: PhantomData,
         };
         let header = header(boxed.ptr.as_ptr());
@@ -302,7 +302,7 @@ impl GcBox<dyn Any + Send> {
         let Some(concrete) = any.downcast_ref::<T>() else { return None; };
         let meta = Metadata::new::<T>(concrete);
         let boxed = GcBox {
-            ptr: unsafe { NonNull::new_unchecked(concrete as *const _ as *mut u8) },
+            ptr: unsafe { NonNull::new_unchecked(concrete as *const _ as *mut ()) },
             _marker: PhantomData,
         };
         let header = header(boxed.ptr.as_ptr());
@@ -325,7 +325,7 @@ impl GcBox<dyn Any + Send + Sync> {
         let Some(concrete) = any.downcast_ref::<T>() else { return None; };
         let meta = Metadata::new::<T>(concrete);
         let boxed = GcBox {
-            ptr: unsafe { NonNull::new_unchecked(concrete as *const _ as *mut u8) },
+            ptr: unsafe { NonNull::new_unchecked(concrete as *const _ as *mut ()) },
             _marker: PhantomData,
         };
         let header = header(boxed.ptr.as_ptr());
@@ -349,7 +349,7 @@ where
         let meta = Metadata::new::<T>(empty);
         let value_layout = unsafe { Layout::for_value_raw(empty) };
         let (layout, value_offset) = Layout::new::<Metadata>().extend(value_layout).unwrap();
-        let ptr: NonNull<u8> = alloc.allocate(layout)?.cast();
+        let ptr: NonNull<()> = alloc.allocate(layout)?.cast();
         unsafe {
             let ptr = NonNull::new_unchecked(ptr.as_ptr().add(value_offset));
             let boxed = Self {
@@ -697,8 +697,8 @@ where
     }
 }
 
-fn header(ptr: *mut u8) -> *mut Metadata {
-    unsafe { ptr.sub(mem::size_of::<Metadata>()).cast() }
+fn header(ptr: *mut ()) -> *mut Metadata {
+    unsafe { ptr.byte_sub(mem::size_of::<Metadata>()).cast() }
 }
 
 /// This is a marker type used to indicate that the data pointed to by a GcBox has been

library/alloc/src/lib.rs

@@ -6,6 +6,7 @@
 #![feature(slice_ptr_len)]
 // Used for access to pointer metadata
 #![feature(ptr_metadata)]
+#![feature(pointer_byte_offsets)]
 // Used for access to size/alignment information for raw pointers
 #![feature(layout_for_ptr)]
 // Used for the Unsize marker trait