Merge pull request #3311 from crystal-lang/feature/fix_proc_extern_st…

…ructs Compiler: respect C ABI for procs and top-level methods that involve extern structs
crystal-lang · Sep 15, 2016 · a967913 · a967913
2 parents 1f3e8b0 + f160b90
commit a967913
Showing 5 changed files with 608 additions and 39 deletions.
diff --git a/spec/compiler/codegen/extern_spec.cr b/spec/compiler/codegen/extern_spec.cr
@@ -142,4 +142,404 @@ describe "Codegen: extern struct" do
       foo.get
       )).to_i.should eq(42)
   end
+
+  # These specs *should* also work for 32 bits, but for now we'll
+  # make sure they work in 64 bits (they probably work in 32 bits too,
+  # it's just that the specs need to be a bit different)
+  {% if flag?(:x86_64) %}
+    it "codegens proc that takes an extern struct with C ABI" do
+      test_c(
+        %(
+            struct Struct {
+              int x;
+              int y;
+            };
+
+            void foo(struct Struct (*callback)(struct Struct)) {
+              struct Struct s;
+              s.x = 1;
+              s.y = 2;
+              callback(s);
+            }
+          ),
+        %(
+            lib LibMylib
+              struct Struct
+                x : Int32
+                y : Int32
+              end
+
+              alias Callback = Struct ->
+
+              fun foo(callback : Callback) : LibMylib::Struct
+            end
+
+            class Global
+              @@x = 0
+              @@y = 0
+
+              def self.x=(@@x)
+              end
+
+              def self.y=(@@y)
+              end
+
+              def self.x
+                @@x
+              end
+
+              def self.y
+                @@y
+              end
+            end
+
+            LibMylib.foo(->(s) {
+              Global.x = s.x
+              Global.y = s.y
+            })
+
+            Global.x + Global.y
+          ), &.to_i.should eq(3))
+    end
+
+    it "codegens proc that takes an extern struct with C ABI (2)" do
+      test_c(
+        %(
+            struct Struct {
+              int x;
+              int y;
+            };
+
+            void foo(struct Struct (*callback)(int, struct Struct, int)) {
+              struct Struct s;
+              s.x = 1;
+              s.y = 2;
+              callback(10, s, 20);
+            }
+          ),
+        %(
+            lib LibMylib
+              struct Struct
+                x : Int32
+                y : Int32
+              end
+
+              alias Callback = Int32, Struct, Int32 ->
+
+              fun foo(callback : Callback) : LibMylib::Struct
+            end
+
+            class Global
+              @@x = 0
+              @@y = 0
+
+              def self.x=(@@x)
+              end
+
+              def self.y=(@@y)
+              end
+
+              def self.x
+                @@x
+              end
+
+              def self.y
+                @@y
+              end
+            end
+
+            LibMylib.foo(->(x, s, y) {
+              Global.x = s.x + x
+              Global.y = s.y + y
+            })
+
+            Global.x + Global.y
+          ), &.to_i.should eq(33))
+    end
+
+    it "codegens proc that takes an extern struct with C ABI, callback returns nil" do
+      test_c(
+        %(
+            struct Struct {
+              int x;
+              int y;
+            };
+
+            void foo(void (*callback)(struct Struct)) {
+              struct Struct s;
+              s.x = 1;
+              s.y = 2;
+              callback(s);
+            }
+          ),
+        %(
+            lib LibMylib
+              struct Struct
+                x : Int32
+                y : Int32
+              end
+
+              alias Callback = Struct ->
+
+              fun foo(callback : Callback) : LibMylib::Struct
+            end
+
+            class Global
+              @@x = 0
+              @@y = 0
+
+              def self.x=(@@x)
+              end
+
+              def self.y=(@@y)
+              end
+
+              def self.x
+                @@x
+              end
+
+              def self.y
+                @@y
+              end
+            end
+
+            LibMylib.foo(->(s) {
+              Global.x = s.x
+              Global.y = s.y
+              nil
+            })
+
+            Global.x + Global.y
+          ), &.to_i.should eq(3))
+    end
+
+    it "codegens proc that takes and returns an extern struct with C ABI" do
+      test_c(
+        %(
+            struct Struct {
+              int x;
+              int y;
+            };
+
+            struct Struct foo(struct Struct (*callback)(struct Struct)) {
+              struct Struct s;
+              s.x = 1;
+              s.y = 2;
+              return callback(s);
+            }
+          ),
+        %(
+            lib LibMylib
+              struct Struct
+                x : Int32
+                y : Int32
+              end
+
+              alias Callback = Struct -> Struct
+
+              fun foo(callback : Callback) : LibMylib::Struct
+            end
+
+            class Global
+              @@x = 0
+              @@y = 0
+
+              def self.x=(@@x)
+              end
+
+              def self.y=(@@y)
+              end
+
+              def self.x
+                @@x
+              end
+
+              def self.y
+                @@y
+              end
+            end
+
+            s2 = LibMylib.foo(->(s) {
+              Global.x = s.x
+              Global.y = s.y
+              s.x = 100
+              s.y = 200
+              s
+            })
+
+            Global.x + Global.y + s2.x + s2.y
+          ), &.to_i.should eq(303))
+    end
+
+    it "codegens proc that takes and returns an extern struct with C ABI" do
+      test_c(
+        %(
+            struct Struct {
+              int x;
+              int y;
+            };
+
+            struct Struct foo(struct Struct (*callback)(int, int)) {
+              return callback(10, 20);
+            }
+          ),
+        %(
+            lib LibMylib
+              struct Struct
+                x : Int32
+                y : Int32
+              end
+
+              alias Callback = Int32, Int32 -> Struct
+
+              fun foo(callback : Callback) : LibMylib::Struct
+            end
+
+            s2 = LibMylib.foo(->(x, y) {
+              s = LibMylib::Struct.new
+              s.x = x
+              s.y = y
+              s
+            })
+
+            s2.x + s2.y
+          ), &.to_i.should eq(30))
+    end
+
+    it "codegens proc that takes and returns an extern struct with sret" do
+      test_c(
+        %(
+            struct Struct {
+              long x;
+              long y;
+              long z;
+            };
+
+            struct Struct foo(struct Struct (*callback)(struct Struct)) {
+              struct Struct s;
+              s.x = 1;
+              s.y = 2;
+              s.z = 3;
+              return callback(s);
+            }
+          ),
+        %(
+            lib LibMylib
+              struct Struct
+                x : Int64
+                y : Int64
+                z : Int64
+              end
+
+              alias Callback = Struct -> Struct
+
+              fun foo(callback : Callback) : LibMylib::Struct
+            end
+
+            class Global
+              @@x = 0
+
+              def self.x=(@@x)
+              end
+
+              def self.x
+                @@x
+              end
+            end
+
+            s2 = LibMylib.foo(->(s) {
+              Global.x += s.x
+              Global.x += s.y
+              Global.x += s.z
+              s
+            })
+
+            Global.x += s2.x
+            Global.x += s2.y
+            Global.x += s2.z
+            Global.x.to_i32
+          ), &.to_i.should eq(12))
+    end
+
+    it "doesn't crash with proc with extern struct that's a closure" do
+      codegen(%(
+          lib LibMylib
+            struct Struct
+              x : Int64
+              y : Int64
+              z : Int64
+            end
+          end
+
+          a = 1
+          f = ->(s : LibMylib::Struct) {
+            a
+          }
+
+          s = LibMylib::Struct.new
+          f.call(s)
+          ))
+    end
+
+    it "invokes proc with extern struct" do
+      run(%(
+          lib LibMylib
+            struct Struct
+              x : Int32
+              y : Int32
+            end
+          end
+
+          class Global
+            @@x = 0
+
+            def self.x=(@@x)
+            end
+
+            def self.x
+              @@x
+            end
+          end
+
+          f = ->(s : LibMylib::Struct) {
+            Global.x += s.x
+            Global.x += s.y
+          }
+
+          s = LibMylib::Struct.new
+          s.x = 10
+          s.y = 20
+          f.call(s)
+
+          Global.x
+          )).to_i.should eq(30)
+    end
+
+    it "invokes proc with extern struct with sret" do
+      run(%(
+          lib LibMylib
+            struct Struct
+              x : Int32
+              y : Int32
+              z : Int32
+              w : Int32
+              a : Int32
+            end
+          end
+
+          f = ->{
+            s = LibMylib::Struct.new
+            s.x = 1
+            s.y = 2
+            s.z = 3
+            s.w = 4
+            s.a = 5
+            s
+          }
+
+          s = f.call
+          s.x + s.y + s.z + s.w + s.a
+          )).to_i.should eq(15)
+    end
+  {% end %}
 end
diff --git a/src/compiler/crystal/codegen/ast.cr b/src/compiler/crystal/codegen/ast.cr
@@ -33,6 +33,8 @@ module Crystal
   end
 
   class Def
+    property abi_info : LLVM::ABI::FunctionType?
+
     def mangled_name(program, self_type)
       name = String.build do |str|
         str << "*"
@@ -91,22 +93,70 @@ module Crystal
       false
     end
 
+    def call_convention
+      nil
+    end
+
+    @considered_external : Bool? = nil
+    setter considered_external
+
     # Returns `self` as an `External` if this Def must be considered
     # an external in the codegen, meaning we need to respect the C ABI.
-    # The only case where this is not true if for LLVM instrinsics.
-    # For example overflow intrincis return a tuple, like {i32, i1}:
-    # in C ABI that is represented as i64, but we need to keep the original
-    # type here, respecting LLVM types, not the C ABI.
     def considered_external?
-      if self.is_a?(External) && !self.real_name.starts_with?("llvm.")
-        self
-      else
-        nil
+      if @considered_external.nil?
+        @considered_external = compute_considered_external
       end
+
+      @considered_external ? self : nil
     end
-  end
 
-  class External
-    property abi_info : LLVM::ABI::FunctionType?
+    private def compute_considered_external
+      # One case where this is not true if for LLVM instrinsics.
+      # For example overflow intrincis return a tuple, like {i32, i1}:
+      # in C ABI that is represented as i64, but we need to keep the original
+      # type here, respecting LLVM types, not the C ABI.
+      if self.is_a?(External)
+        return !self.real_name.starts_with?("llvm.")
+      end
+
+      # Another case is when an argument is an external struct, in which
+      # case we must respect the C ABI (this applies to Crystal methods
+      # and procs too)
+
+      # Only applicable to procs (no owner) for now
+      owner = @owner
+      if owner
+        return false
+      end
+
+      proc_considered_external?
+    end
+
+    def proc_considered_external?
+      # We use C ABI if:
+      # - all arguments are allowed in lib calls (because then it can be passed to C)
+      # - at least one argument type, or the return type, is an extern struct
+      found_extern = false
+
+      if (type = self.type?)
+        type = type.remove_alias
+        if type.extern?
+          found_extern = true
+        elsif !type.void? && !type.nil_type? && !type.allowed_in_lib?
+          return false
+        end
+      end
+
+      args.each do |arg|
+        arg_type = arg.type.remove_alias
+        if arg_type.extern?
+          found_extern = true
+        elsif !arg_type.allowed_in_lib?
+          return false
+        end
+      end
+
+      found_extern
+    end
   end
 end
diff --git a/src/compiler/crystal/codegen/call.cr b/src/compiler/crystal/codegen/call.cr
@@ -197,17 +197,7 @@ class Crystal::CodeGenVisitor
       abi_arg_type = abi_info.arg_types[i]
       case abi_arg_type.kind
       when LLVM::ABI::ArgKind::Direct
-        if (cast = abi_arg_type.cast) && !arg.is_a?(NilLiteral)
-          final_value = alloca cast
-          final_value_casted = bit_cast final_value, LLVM::VoidPointer
-          gep_call_arg = bit_cast gep(call_arg, 0, 0), LLVM::VoidPointer
-          size = @abi.size(abi_arg_type.type)
-          align = @abi.align(abi_arg_type.type)
-          memcpy(final_value_casted, gep_call_arg, int32(size), int32(align), int1(0))
-          call_arg = load final_value
-        else
-          # Keep same call arg
-        end
+        call_arg = codegen_direct_abi_call(call_arg, abi_arg_type) unless arg.is_a?(NilLiteral)
         call_args << call_arg
       when LLVM::ABI::ArgKind::Indirect
         # Pass argument as is (will be passed byval)
@@ -222,6 +212,21 @@ class Crystal::CodeGenVisitor
     {call_args, has_out}
   end
 
+  def codegen_direct_abi_call(call_arg, abi_arg_type)
+    if cast = abi_arg_type.cast
+      final_value = alloca cast
+      final_value_casted = bit_cast final_value, LLVM::VoidPointer
+      gep_call_arg = bit_cast gep(call_arg, 0, 0), LLVM::VoidPointer
+      size = @abi.size(abi_arg_type.type)
+      align = @abi.align(abi_arg_type.type)
+      memcpy(final_value_casted, gep_call_arg, int32(size), int32(align), int1(0))
+      call_arg = load final_value
+    else
+      # Keep same call arg
+    end
+    call_arg
+  end
+
   def codegen_call_with_block(node, block, self_type, call_args)
     with_cloned_context do |old_block_context|
       context.vars = old_block_context.vars.dup
@@ -515,7 +520,7 @@ class Crystal::CodeGenVisitor
   def set_call_attributes_external(node, target_def)
     abi_info = call_abi_info(target_def, node)
 
-    sret = abi_info.return_type.attr == LLVM::Attribute::StructRet
+    sret = sret?(abi_info)
     arg_offset = 1
     arg_offset += 1 if sret
 
@@ -544,11 +549,23 @@ class Crystal::CodeGenVisitor
 
   # This is for function pointer calls and exception handler re-raise
   def set_call_attributes(node, target_def, self_type, is_closure, fun_type)
+    abi_info = target_def.abi_info if target_def
+
     arg_offset = is_closure ? 2 : 1
     arg_types = fun_type.try(&.arg_types) || target_def.try &.args.map &.type
     arg_types.try &.each_with_index do |arg_type, i|
-      next unless arg_type.passed_by_value?
-      @last.add_instruction_attribute(i + arg_offset, LLVM::Attribute::ByVal)
+      if abi_info && (abi_arg_type = abi_info.arg_types[i]?)
+        if (attr = abi_arg_type.attr)
+          @last.add_instruction_attribute(i + arg_offset, attr)
+        end
+      else
+        next unless arg_type.passed_by_value?
+        @last.add_instruction_attribute(i + arg_offset, LLVM::Attribute::ByVal)
+      end
     end
   end
+
+  def sret?(abi_info)
+    abi_info.return_type.attr == LLVM::Attribute::StructRet
+  end
 end
diff --git a/src/compiler/crystal/codegen/fun.cr b/src/compiler/crystal/codegen/fun.cr
@@ -114,7 +114,8 @@ class Crystal::CodeGenVisitor
         accept target_def.body
 
         set_current_debug_location target_def.end_location if @debug
-        codegen_return target_def.body.type?
+
+        codegen_return(target_def)
 
         br_from_alloca_to_entry
       end
@@ -134,8 +135,30 @@ class Crystal::CodeGenVisitor
     end
   end
 
+  def codegen_return(target_def : Def)
+    # Check if this def is considered external and the return
+    # value must be either casted or passed by sret
+    if target_def.considered_external? && (abi_info = target_def.abi_info)
+      ret_type = abi_info.return_type
+      if cast = ret_type.cast
+        casted_last = bit_cast @last, cast.pointer
+        last = load casted_last
+        ret last
+        return
+      end
+
+      if (attr = ret_type.attr) && attr == LLVM::Attribute::StructRet
+        store load(@last), context.fun.params[0]
+        ret
+        return
+      end
+    end
+
+    codegen_return target_def.body.type?
+  end
+
   def codegen_fun_signature(mangled_name, target_def, self_type, is_fun_literal, is_closure)
-    if external = target_def.considered_external?
+    if !is_closure && (external = target_def.considered_external?)
       codegen_fun_signature_external(mangled_name, external)
     else
       codegen_fun_signature_non_external(mangled_name, target_def, self_type, is_fun_literal, is_closure)
@@ -272,15 +295,15 @@ class Crystal::CodeGenVisitor
     args
   end
 
-  def abi_info(external : External)
+  def abi_info(external : Def)
     external.abi_info ||= begin
       llvm_args_types = external.args.map { |arg| llvm_c_type(arg.type) }
       llvm_return_type = llvm_c_return_type(external.type)
       @abi.abi_info(llvm_args_types, llvm_return_type, !llvm_return_type.void?)
     end
   end
 
-  def abi_info(external : External, node : Call)
+  def abi_info(external : Def, node : Call)
     llvm_args_types = node.args.map { |arg| llvm_c_type(arg.type) }
     llvm_return_type = llvm_c_return_type(external.type)
     @abi.abi_info(llvm_args_types, llvm_return_type, !llvm_return_type.void?)
@@ -330,13 +353,17 @@ class Crystal::CodeGenVisitor
   def create_local_copy_of_fun_args(target_def, self_type, args, is_fun_literal, is_closure)
     offset = is_closure ? 1 : 0
 
+    abi_info = target_def.abi_info
+    sret = abi_info && sret?(abi_info)
+    offset += 1 if sret
+
     target_def_vars = target_def.vars
     args.each_with_index do |arg, i|
       param = context.fun.params[i + offset]
       if !is_fun_literal && (i == 0 && self_type.passed_as_self?)
         # here self is already in context.vars
       else
-        create_local_copy_of_arg(target_def_vars, arg, param)
+        create_local_copy_of_arg(target_def, target_def_vars, arg, param)
       end
     end
   end
@@ -349,11 +376,11 @@ class Crystal::CodeGenVisitor
     end
 
     target_def.args.each_with_index do |arg, i|
-      create_local_copy_of_arg(target_def.vars, arg, call_args[args_base_index + i])
+      create_local_copy_of_arg(target_def, target_def.vars, arg, call_args[args_base_index + i])
     end
   end
 
-  def create_local_copy_of_arg(target_def_vars, arg, value)
+  def create_local_copy_of_arg(target_def, target_def_vars, arg, value)
     # An argument name can be "_" in the case of a captured block,
     # and we must ignore these
     return if arg.name == "_"
@@ -366,15 +393,25 @@ class Crystal::CodeGenVisitor
     if closure_var = context.vars[arg.name]?
       pointer = closure_var.pointer
     else
-      # We don't need to create a copy of the argument if it's never
-      # assigned a value inside the function.
-      needs_copy = target_def_var.try &.assigned_to?
-      if needs_copy && !arg.special_var?
+      # If it's an extern struct on a def that must be codegened with C ABI
+      # compatibility, and it's not passed byval, we must cast the value
+      if target_def.considered_external? && arg.type.extern? && !value.attributes.by_val?
         pointer = alloca(llvm_type(var_type), arg.name)
+        casted_pointer = bit_cast pointer, value.type.pointer
+        store value, casted_pointer
         context.vars[arg.name] = LLVMVar.new(pointer, var_type)
-      else
-        context.vars[arg.name] = LLVMVar.new(value, var_type, true)
         return
+      else
+        # We don't need to create a copy of the argument if it's never
+        # assigned a value inside the function.
+        needs_copy = target_def_var.try &.assigned_to?
+        if needs_copy && !arg.special_var?
+          pointer = alloca(llvm_type(var_type), arg.name)
+          context.vars[arg.name] = LLVMVar.new(pointer, var_type)
+        else
+          context.vars[arg.name] = LLVMVar.new(value, var_type, true)
+          return
+        end
       end
     end
 

diff --git a/src/compiler/crystal/codegen/primitives.cr b/src/compiler/crystal/codegen/primitives.cr
@@ -633,9 +633,26 @@ class Crystal::CodeGenVisitor
     phi_value = Phi.open(self, node, @needs_value) do |phi|
       position_at_end ctx_is_null_block
       real_fun_ptr = bit_cast fun_ptr, llvm_proc_type(context.type)
-      value = codegen_call_or_invoke(node, target_def, nil, real_fun_ptr, args, true, target_def.type, false, proc_type)
+
+      # When invoking a Proc that has extern structs as arguments or return type, it's tricky:
+      # closures are never generated with C ABI because C doesn't support closures.
+      # But non-closures use C ABI, so if the target Proc is not a closure we cast the
+      # arguments according to the ABI.
+      # For this we temporarily set the target_def's `abi_info` and `considered_external`
+      # properties for the non-closure branch, and then reset it.
+      if target_def.proc_considered_external?
+        null_fun_ptr, null_args = codegen_extern_primitive_proc_call(target_def, args, fun_ptr)
+      else
+        null_fun_ptr, null_args = real_fun_ptr, args
+      end
+
+      value = codegen_call_or_invoke(node, target_def, nil, null_fun_ptr, null_args, true, target_def.type, false, proc_type)
       phi.add value, node.type
 
+      # Reset abi_info + considered_external so the closure part is generated as usual
+      target_def.abi_info = nil
+      target_def.considered_external = nil
+
       position_at_end ctx_is_not_null_block
       real_fun_ptr = bit_cast fun_ptr, llvm_closure_type(context.type)
       args.insert(0, ctx_ptr)
@@ -647,6 +664,54 @@ class Crystal::CodeGenVisitor
     phi_value
   end
 
+  def codegen_extern_primitive_proc_call(target_def, args, fun_ptr)
+    null_fun_types = [] of LLVM::Type
+
+    null_args = [] of LLVM::Value
+    abi_info = abi_info(target_def)
+
+    if abi_info.return_type.attr == LLVM::Attribute::StructRet
+      sret_value = @sret_value = alloca abi_info.return_type.type
+      null_args << sret_value
+      null_fun_types << abi_info.return_type.type.pointer
+      null_fun_return_type = LLVM::Void
+    else
+      if cast = abi_info.return_type.cast
+        null_fun_return_type = cast
+      else
+        null_fun_return_type = abi_info.return_type.type
+      end
+    end
+
+    target_def.args.each_with_index do |arg, index|
+      call_arg = args[index]
+
+      abi_arg_type = abi_info.arg_types[index]
+      case abi_arg_type.kind
+      when LLVM::ABI::ArgKind::Direct
+        call_arg = codegen_direct_abi_call(call_arg, abi_arg_type)
+        if cast = abi_arg_type.cast
+          null_fun_types << cast
+        else
+          null_fun_types << abi_arg_type.type
+        end
+        null_args << call_arg
+      when LLVM::ABI::ArgKind::Indirect
+        # Pass argument as is (will be passed byval)
+        null_args << call_arg
+        null_fun_types << abi_arg_type.type.pointer
+      when LLVM::ABI::ArgKind::Ignore
+        # Ignore
+      end
+    end
+
+    null_fun_llvm_type = LLVM::Type.function(null_fun_types, null_fun_return_type)
+    null_fun_ptr = bit_cast fun_ptr, null_fun_llvm_type.pointer
+    target_def.considered_external = true
+
+    {null_fun_ptr, null_args}
+  end
+
   def codegen_primitive_pointer_diff(node, target_def, call_args)
     p0 = ptr2int(call_args[0], LLVM::Int64)
     p1 = ptr2int(call_args[1], LLVM::Int64)