From e3eb178f213ed66fce06286b586b687e3a6bf65c Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Wed, 28 Feb 2024 03:50:51 +0800 Subject: [PATCH] Fix `Proc#call` that takes and returns large extern structs by value (#14323) --- spec/compiler/codegen/extern_spec.cr | 23 +++++++++++++-- spec/std/llvm/aarch64_spec.cr | 2 +- spec/std/llvm/x86_64_abi_spec.cr | 44 +++++++++++++++++++++++----- src/compiler/crystal/codegen/call.cr | 7 +++++ src/compiler/crystal/codegen/fun.cr | 5 +++- src/llvm/abi/x86_win64.cr | 2 +- 6 files changed, 71 insertions(+), 12 deletions(-) diff --git a/spec/compiler/codegen/extern_spec.cr b/spec/compiler/codegen/extern_spec.cr index 6efd7a44c16b..e069e81a75fe 100644 --- a/spec/compiler/codegen/extern_spec.cr +++ b/spec/compiler/codegen/extern_spec.cr @@ -164,10 +164,29 @@ describe "Codegen: extern struct" do )).to_i.should eq(3) end + it "codegens proc that takes and returns large extern struct by value" do + run(<<-CRYSTAL).to_i.should eq(149) + @[Extern] + struct Foo + @unused = uninitialized Int64 + + def initialize(@x : Int32, @y : Int32, @z : Int32) + end + end + + f = ->(foo : Foo) { + Foo.new(foo.@x, foo.@y &* 2, foo.@z &* 3) + } + + foo = f.call(Foo.new(100, 20, 3)) + foo.@x &+ foo.@y &+ foo.@z + CRYSTAL + 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) %} + {% if flag?(:x86_64) || flag?(:aarch64) %} it "codegens proc that takes an extern struct with C ABI" do test_c( %( @@ -427,7 +446,7 @@ describe "Codegen: extern struct" do ), &.to_i.should eq(30)) end - pending_win32 "codegens proc that takes and returns an extern struct with sret" do + it "codegens proc that takes and returns an extern struct with sret" do test_c( %( struct Struct { diff --git a/spec/std/llvm/aarch64_spec.cr b/spec/std/llvm/aarch64_spec.cr index a7303fd2e861..41a308b480ec 100644 --- a/spec/std/llvm/aarch64_spec.cr +++ b/spec/std/llvm/aarch64_spec.cr @@ -133,7 +133,7 @@ class LLVM::ABI info.return_type.should eq(ArgType.direct(str, cast: ctx.int64.array(2))) end - test "does with structs between 64 and 128 bits" do |abi, ctx| + test "does with structs larger than 128 bits" do |abi, ctx| str = ctx.struct([ctx.int64, ctx.int64, ctx.int8]) arg_types = [str] return_type = str diff --git a/spec/std/llvm/x86_64_abi_spec.cr b/spec/std/llvm/x86_64_abi_spec.cr index 7c0344769aa3..0ba644cefa01 100644 --- a/spec/std/llvm/x86_64_abi_spec.cr +++ b/spec/std/llvm/x86_64_abi_spec.cr @@ -5,17 +5,17 @@ require "llvm" LLVM.init_x86 {% end %} -private def abi - triple = LLVM.default_target_triple.gsub(/^(.+?)-/, "x86_64-") +private def abi(win64 = false) + triple = win64 ? "x86_64-windows-msvc" : LLVM.default_target_triple.gsub(/^(.+?)-/, "x86_64-") target = LLVM::Target.from_triple(triple) machine = target.create_target_machine(triple) machine.enable_global_isel = false - LLVM::ABI::X86_64.new(machine) + win64 ? LLVM::ABI::X86_Win64.new(machine) : LLVM::ABI::X86_64.new(machine) end -private def test(msg, &block : LLVM::ABI, LLVM::Context ->) - it msg do - abi = abi() +private def test(msg, *, win64 = false, file = __FILE__, line = __LINE__, &block : LLVM::ABI, LLVM::Context ->) + it msg, file: file, line: line do + abi = abi(win64) ctx = LLVM::Context.new block.call(abi, ctx) end @@ -133,7 +133,37 @@ class LLVM::ABI info.return_type.should eq(ArgType.direct(str, cast: ctx.struct([ctx.int64, ctx.int64]))) end - test "does with structs between 64 and 128 bits" do |abi, ctx| + test "does with structs larger than 128 bits" do |abi, ctx| + str = ctx.struct([ctx.int64, ctx.int64, ctx.int8]) + arg_types = [str] + return_type = str + + info = abi.abi_info(arg_types, return_type, true, ctx) + info.arg_types.size.should eq(1) + + info.arg_types[0].should eq(ArgType.indirect(str, Attribute::ByVal)) + info.return_type.should eq(ArgType.indirect(str, Attribute::StructRet)) + end + end + {% end %} + end + + describe X86_Win64 do + {% if LibLLVM::BUILT_TARGETS.includes?(:x86) %} + describe "abi_info" do + test "does with structs between 64 and 128 bits", win64: true do |abi, ctx| + str = ctx.struct([ctx.int64, ctx.int16]) + arg_types = [str] + return_type = str + + info = abi.abi_info(arg_types, return_type, true, ctx) + info.arg_types.size.should eq(1) + + info.arg_types[0].should eq(ArgType.indirect(str, Attribute::ByVal)) + info.return_type.should eq(ArgType.indirect(str, Attribute::StructRet)) + end + + test "does with structs larger than 128 bits", win64: true do |abi, ctx| str = ctx.struct([ctx.int64, ctx.int64, ctx.int8]) arg_types = [str] return_type = str diff --git a/src/compiler/crystal/codegen/call.cr b/src/compiler/crystal/codegen/call.cr index d0937cfff40c..1dca1f4e4b7b 100644 --- a/src/compiler/crystal/codegen/call.cr +++ b/src/compiler/crystal/codegen/call.cr @@ -597,13 +597,20 @@ class Crystal::CodeGenVisitor abi_info = abi_info(target_def) end + sret = abi_info && sret?(abi_info) arg_offset = is_closure ? 2 : 1 + arg_offset += 1 if sret + arg_types = fun_type.try(&.arg_types) || target_def.try &.args.map &.type arg_types.try &.each_with_index do |arg_type, i| if abi_info && (abi_arg_type = abi_info.arg_types[i]?) && (attr = abi_arg_type.attr) @last.add_instruction_attribute(i + arg_offset, attr, llvm_context, abi_arg_type.type) end end + + if abi_info && sret + @last.add_instruction_attribute(1, LLVM::Attribute::StructRet, llvm_context, abi_info.return_type.type) + end end def sret?(abi_info) diff --git a/src/compiler/crystal/codegen/fun.cr b/src/compiler/crystal/codegen/fun.cr index 784eba83b9b9..7d8ddf065c21 100644 --- a/src/compiler/crystal/codegen/fun.cr +++ b/src/compiler/crystal/codegen/fun.cr @@ -535,11 +535,14 @@ class Crystal::CodeGenVisitor value = pointer2 end else + abi_info = target_def.abi_info? ? abi_info(target_def) : nil + sret = abi_info && sret?(abi_info) + # 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.c_calling_convention? && arg.type.extern? && !context.fun.attributes(index + 1).by_val? # ... unless it's passed indirectly (ie. as a pointer to memory allocated by the caller) - if target_def.abi_info? && abi_info(target_def).arg_types[index].kind.indirect? + if abi_info.try &.arg_types[index - (sret ? 1 : 0)].kind.indirect? value = declare_debug_for_function_argument(arg.name, var_type, index + 1, value, location) unless target_def.naked? context.vars[arg.name] = LLVMVar.new(value, var_type) else diff --git a/src/llvm/abi/x86_win64.cr b/src/llvm/abi/x86_win64.cr index 69e9b0823ba9..5a9a07bdb31e 100644 --- a/src/llvm/abi/x86_win64.cr +++ b/src/llvm/abi/x86_win64.cr @@ -12,7 +12,7 @@ class LLVM::ABI::X86_Win64 < LLVM::ABI::X86 when 2 then ArgType.direct(t, context.int16) when 4 then ArgType.direct(t, context.int32) when 8 then ArgType.direct(t, context.int64) - else ArgType.indirect(t, nil) + else ArgType.indirect(t, LLVM::Attribute::ByVal) end else non_struct(t, context)