Fix use of GEP (was using byte offset, not index)

Cargo.lock

@@ -833,6 +833,7 @@ dependencies = [
  "indoc",
  "inkwell",
  "inlinable_string",
+ "libc",
  "maplit",
  "pretty_assertions",
  "quickcheck",

compiler/gen/Cargo.toml

@@ -54,3 +54,4 @@ quickcheck = "0.8"
 quickcheck_macros = "0.8"
 tokio = { version = "0.2", features = ["blocking", "fs", "sync", "rt-threaded"] }
 bumpalo = { version = "3.2", features = ["collections"] }
+libc = "0.2"

compiler/gen/src/crane/build.rs

@@ -351,6 +351,7 @@ pub fn build_expr<'a, B: Backend>(
                 let bytes_len = str_literal.len() + 1/* TODO drop the +1 when we have structs and this is no longer a NUL-terminated CString.*/;
                 let size = builder.ins().iconst(types::I64, bytes_len as i64);
                 let ptr = call_malloc(env, module, builder, size);
+                // TODO check if malloc returned null; if so, runtime error for OOM!
                 let mem_flags = MemFlags::new();
 
                 // Store the bytes from the string literal in the array
@@ -389,6 +390,8 @@ pub fn build_expr<'a, B: Backend>(
                 let bytes_len = elem_bytes as usize * elems.len();
                 let size = builder.ins().iconst(types::I64, bytes_len as i64);
                 let elems_ptr = call_malloc(env, module, builder, size);
+
+                // TODO check if malloc returned null; if so, runtime error for OOM!
                 let mem_flags = MemFlags::new();
 
                 // Copy the elements from the literal into the array
@@ -1044,6 +1047,8 @@ fn clone_list<B: Backend>(
     // Allocate space for the new array that we'll copy into.
     let new_elems_ptr = call_malloc(env, module, builder, size);
 
+    // TODO check if malloc returned null; if so, runtime error for OOM!
+
     // Either memcpy or deep clone the array elements
     if elem_layout.safe_to_memcpy() {
         // Copy the bytes from the original array into the new

compiler/gen/src/llvm/build.rs

@@ -208,20 +208,23 @@ pub fn build_expr<'a, 'ctx, 'env>(
             } else {
                 let ctx = env.context;
                 let builder = env.builder;
-                let bytes_len = str_literal.len() + 1/* TODO drop the +1 when we have structs and this is no longer a NUL-terminated CString.*/;
+                let str_len = str_literal.len() + 1/* TODO drop the +1 when we have structs and this is no longer a NUL-terminated CString.*/;
 
                 let byte_type = ctx.i8_type();
                 let nul_terminator = byte_type.const_zero();
-                let len = ctx.i32_type().const_int(bytes_len as u64, false);
+                let len_val = ctx.i32_type().const_int(str_len as u64, false);
                 let ptr = env
                     .builder
-                    .build_array_malloc(ctx.i8_type(), len, "str_ptr")
+                    .build_array_malloc(ctx.i8_type(), len_val, "str_ptr")
                     .unwrap();
 
+                // TODO check if malloc returned null; if so, runtime error for OOM!
+
                 // Copy the bytes from the string literal into the array
                 for (index, byte) in str_literal.bytes().enumerate() {
-                    let index = ctx.i32_type().const_int(index as u64, false);
+                    let index_val = ctx.i32_type().const_int(index as u64, false);
-                    let elem_ptr = unsafe { builder.build_gep(ptr, &[index], "byte") };
+                    let elem_ptr =
+                        unsafe { builder.build_in_bounds_gep(ptr, &[index_val], "byte") };
 
                     builder.build_store(elem_ptr, byte_type.const_int(byte as u64, false));
                 }
@@ -229,8 +232,9 @@ pub fn build_expr<'a, 'ctx, 'env>(
                 // Add a NUL terminator at the end.
                 // TODO: Instead of NUL-terminating, return a struct
                 // with the pointer and also the length and capacity.
-                let index = ctx.i32_type().const_int(bytes_len as u64 - 1, false);
+                let index_val = ctx.i32_type().const_int(str_len as u64 - 1, false);
-                let elem_ptr = unsafe { builder.build_gep(ptr, &[index], "nul_terminator") };
+                let elem_ptr =
+                    unsafe { builder.build_in_bounds_gep(ptr, &[index_val], "nul_terminator") };
 
                 builder.build_store(elem_ptr, nul_terminator);
 
@@ -260,13 +264,15 @@ pub fn build_expr<'a, 'ctx, 'env>(
                     env.builder
                         .build_array_malloc(elem_type, len, "create_list_ptr")
                         .unwrap()
+
+                    // TODO check if malloc returned null; if so, runtime error for OOM!
                 };
 
                 // Copy the elements from the list literal into the array
                 for (index, elem) in elems.iter().enumerate() {
-                    let offset = ctx.i32_type().const_int(elem_bytes * index as u64, false);
+                    let index_val = ctx.i32_type().const_int(index as u64, false);
-                    let elem_ptr = unsafe { builder.build_gep(ptr, &[offset], "elem") };
+                    let elem_ptr =
-
+                        unsafe { builder.build_in_bounds_gep(ptr, &[index_val], "index") };
                     let val = build_expr(env, &scope, parent, &elem, procs);
 
                     builder.build_store(elem_ptr, val);
@@ -1065,18 +1071,13 @@ fn call_with_args<'a, 'ctx, 'env>(
             // TODO here, check to see if the requested index exceeds the length of the array.
 
             match list_layout {
-                Layout::Builtin(Builtin::List(elem_layout)) => {
+                Layout::Builtin(Builtin::List(_)) => {
                     // Load the pointer to the array data
                     let array_data_ptr = load_list_ptr(builder, wrapper_struct);
-                    let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
-                    let elem_size = env.ptr_int().const_int(elem_bytes, false);
-
-                    // Calculate the offset at runtime by multiplying the index by the size of an element.
-                    let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
 
                     // We already checked the bounds earlier.
                     let elem_ptr = unsafe {
-                        builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem")
+                        builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem")
                     };
 
                     builder.build_load(elem_ptr, "List.get")
@@ -1111,16 +1112,12 @@ fn call_with_args<'a, 'ctx, 'env>(
                 )
             };
 
-            let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
-            let elem_size = env.ptr_int().const_int(elem_bytes, false);
-
             // Calculate the offset at runtime by multiplying the index by the size of an element.
             let elem_index = args[1].0.into_int_value();
-            let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
 
             // We already checked the bounds earlier.
             let elem_ptr =
-                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem") };
+                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem") };
 
             // Mutate the array in-place.
             builder.build_store(elem_ptr, elem);
@@ -1135,7 +1132,7 @@ fn call_with_args<'a, 'ctx, 'env>(
 
             let wrapper_struct = args[0].0.into_struct_value();
             let elem_index = args[1].0.into_int_value();
-            let (elem, elem_layout) = args[2];
+            let (elem, _elem_layout) = args[2];
 
             // Load the usize length
             let _list_len = load_list_len(builder, wrapper_struct);
@@ -1146,15 +1143,9 @@ fn call_with_args<'a, 'ctx, 'env>(
             // Load the pointer to the elements
             let array_data_ptr = load_list_ptr(builder, wrapper_struct);
 
-            let elem_bytes = elem_layout.stack_size(env.ptr_bytes) as u64;
-            let elem_size = env.context.i64_type().const_int(elem_bytes, false);
-
-            // Calculate the offset at runtime by multiplying the index by the size of an element.
-            let offset_bytes = builder.build_int_mul(elem_index, elem_size, "mul_offset");
-
             // We already checked the bounds earlier.
             let elem_ptr =
-                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[offset_bytes], "elem") };
+                unsafe { builder.build_in_bounds_gep(array_data_ptr, &[elem_index], "elem") };
 
             // Mutate the array in-place.
             builder.build_store(elem_ptr, elem);
@@ -1229,6 +1220,8 @@ fn clone_list<'a, 'ctx, 'env>(
         .build_array_malloc(elem_type, list_len, "list_ptr")
         .unwrap();
 
+    // TODO check if malloc returned null; if so, runtime error for OOM!
+
     // Either memcpy or deep clone the array elements
     if elem_layout.safe_to_memcpy() {
         // Copy the bytes from the original array into the new

compiler/gen/src/llvm/convert.rs

@@ -151,7 +151,14 @@ pub fn collection_wrapper<'ctx>(
     let ptr_type_enum = BasicTypeEnum::PointerType(ptr_type);
     let len_type = BasicTypeEnum::IntType(ptr_int(ctx, ptr_bytes));
 
+    // This conditional is based on a constant, so the branch should be optimized away.
+    // The reason for keeping the conditional here is so we can flip the order
+    // of the fields (by changing the constants) without breaking this code.
+    if Builtin::WRAPPER_PTR == 0 {
         ctx.struct_type(&[ptr_type_enum, len_type], false)
+    } else {
+        ctx.struct_type(&[len_type, ptr_type_enum], false)
+    }
 }
 
 pub fn ptr_int(ctx: &Context, ptr_bytes: u32) -> IntType<'_> {

compiler/gen/tests/test_gen.rs

@@ -5,6 +5,7 @@ extern crate indoc;
 
 extern crate bumpalo;
 extern crate inkwell;
+extern crate libc;
 extern crate roc_gen;
 
 mod helpers;
@@ -541,6 +542,11 @@ mod test_gen {
         assert_llvm_evals_to!("[]", &[], &'static [i64], |x| x);
     }
 
+    #[test]
+    fn int_list_literal() {
+        assert_llvm_evals_to!("[ 12, 9, 6, 3 ]", &[12, 9, 6, 3], &'static [i64], |x| x);
+    }
+
     #[test]
     fn head_int_list() {
         assert_evals_to!("List.getUnsafe [ 12, 9, 6, 3 ] 0", 12, i64);

compiler/mono/src/layout.rs

@@ -143,10 +143,12 @@ impl<'a> Builtin<'a> {
     pub const SET_WORDS: u32 = Builtin::MAP_WORDS; // Set is an alias for Map with {} for value
     pub const LIST_WORDS: u32 = 2;
 
-    /// Layout of collection wrapper - a struct of (pointer, length, capacity)
+    /// Layout of collection wrapper for List and Str - a struct of (pointre, length).
+    ///
+    /// We choose this layout (with pointer first) because it's how
+    /// Rust slices are laid out, meaning we can cast to/from them for free.
     pub const WRAPPER_PTR: u32 = 0;
     pub const WRAPPER_LEN: u32 = 1;
-    pub const WRAPPER_CAPACITY: u32 = 2;
 
     pub fn stack_size(&self, pointer_size: u32) -> u32 {
         use Builtin::*;