From 759f6a35cafd3aaad5cc290ee085918ebf400058 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Thu, 4 Jun 2020 14:27:40 +0200 Subject: [PATCH] Fix VaList and disable va_arg for AArch64 va_arg is broken on most platforms, including AArch64 --- spec/compiler/codegen/primitives_spec.cr | 50 +++++++++---------- spec/spec_helper.cr | 6 +-- spec/std/spec_helper.cr | 11 ++++ spec/std/va_list_spec.cr | 40 +++++++++++++++ spec/support/tempfile.cr | 10 ++++ spec/win32_std_spec.cr | 1 + src/compiler/crystal/semantic/main_visitor.cr | 3 -- src/lib_c/aarch64-linux-gnu/c/stdarg.cr | 9 +++- src/lib_c/aarch64-linux-musl/c/stdarg.cr | 14 +++--- src/va_list.cr | 12 ++++- 10 files changed, 114 insertions(+), 42 deletions(-) create mode 100644 spec/std/va_list_spec.cr diff --git a/spec/compiler/codegen/primitives_spec.cr b/spec/compiler/codegen/primitives_spec.cr index 20e768e66086..7d6fba92ec37 100644 --- a/spec/compiler/codegen/primitives_spec.cr +++ b/spec/compiler/codegen/primitives_spec.cr @@ -239,8 +239,8 @@ describe "Code gen: primitives" do end describe "va_arg" do - # On Windows llvm's va_arg instruction works incorrectly. - {% unless flag?(:win32) %} + # On Windows and AArch64 llvm's va_arg instruction works incorrectly. + {% unless flag?(:win32) || flag?(:aarch64) %} it "uses llvm's va_arg instruction" do mod = codegen(%( struct VaList @@ -255,33 +255,33 @@ describe "Code gen: primitives" do str = mod.to_s str.should contain("va_arg %VaList* %list") end - {% end %} - pending_win32 "works with C code" do - test_c( - %( - extern int foo_f(int,...); - int foo() { - return foo_f(3,1,2,3); - } - ), - %( - lib LibFoo - fun foo() : LibC::Int - end + it "works with C code" do + test_c( + %( + extern int foo_f(int,...); + int foo() { + return foo_f(3,1,2,3); + } + ), + %( + lib LibFoo + fun foo() : LibC::Int + end - fun foo_f(count : Int32, ...) : LibC::Int - sum = 0 - VaList.open do |list| - count.times do |i| - sum += list.next(Int32) + fun foo_f(count : Int32, ...) : LibC::Int + sum = 0 + VaList.open do |list| + count.times do |i| + sum += list.next(Int32) + end end + sum end - sum - end - LibFoo.foo - ), &.to_i.should eq(6)) - end + LibFoo.foo + ), &.to_i.should eq(6)) + end + {% end %} end end diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index 0a67b2f9f802..ee121a3a6038 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -257,11 +257,7 @@ def run(code, filename = nil, inject_primitives = true, debug = Crystal::Debug:: end def test_c(c_code, crystal_code, *, file = __FILE__) - with_tempfile("temp_abi.c", "temp_abi.o", file: file) do |c_filename, o_filename| - File.write(c_filename, c_code) - - `#{Crystal::Compiler::CC} #{Process.quote(c_filename)} -c -o #{Process.quote(o_filename)}`.should be_truthy - + with_temp_c_object_file(c_code, file: file) do |o_filename| yield run(%( require "prelude" diff --git a/spec/std/spec_helper.cr b/spec/std/spec_helper.cr index b87590327247..943c90a9bde4 100644 --- a/spec/std/spec_helper.cr +++ b/spec/std/spec_helper.cr @@ -108,3 +108,14 @@ def compile_and_run_source(source, flags = %w(--debug), file = __FILE__) compile_and_run_file(source_file, flags, file: file) end end + +def compile_and_run_source_with_c(c_code, crystal_code, flags = %w(--debug), file = __FILE__) + with_temp_c_object_file(c_code, file: file) do |o_filename| + yield compile_and_run_source(%( + require "prelude" + + @[Link(ldflags: #{o_filename.inspect})] + #{crystal_code} + )) + end +end diff --git a/spec/std/va_list_spec.cr b/spec/std/va_list_spec.cr new file mode 100644 index 000000000000..79347c0638fe --- /dev/null +++ b/spec/std/va_list_spec.cr @@ -0,0 +1,40 @@ +require "./spec_helper" + +describe VaList do + it "works with C code" do + compile_and_run_source_with_c( + %( + #include + extern int foo_f(int,...); + int foo() { + return foo_f(3,1,2,3); + } + + int read_arg(va_list *ap) { + return va_arg(*ap, int); + } + ), + %( + lib LibFoo + fun foo : LibC::Int + fun read_arg(ap : LibC::VaList*) : LibC::Int + end + + fun foo_f(count : LibC::Int, ...) : LibC::Int + sum = 0 + VaList.open do |list| + ap = list.to_unsafe + count.times do |i| + sum += LibFoo.read_arg(pointerof(ap)) + end + end + sum + end + + puts LibFoo.foo + )) do |status, output| + status.success?.should be_true + output.to_i.should eq(6) + end + end +end diff --git a/spec/support/tempfile.cr b/spec/support/tempfile.cr index 0c47ee765e90..42001785d1c8 100644 --- a/spec/support/tempfile.cr +++ b/spec/support/tempfile.cr @@ -43,6 +43,16 @@ def with_temp_executable(name, file = __FILE__) end end +def with_temp_c_object_file(c_code, file = __FILE__) + with_tempfile("temp_c.c", "temp_c.o", file: file) do |c_filename, o_filename| + File.write(c_filename, c_code) + + `#{ENV["CC"]? || "cc"} #{Process.quote(c_filename)} -c -o #{Process.quote(o_filename)}`.should be_truthy + + yield o_filename + end +end + if SPEC_TEMPFILE_CLEANUP at_exit do FileUtils.rm_r(SPEC_TEMPFILE_PATH) if Dir.exists?(SPEC_TEMPFILE_PATH) diff --git a/spec/win32_std_spec.cr b/spec/win32_std_spec.cr index dbf3f13c8a08..2bc3be978063 100644 --- a/spec/win32_std_spec.cr +++ b/spec/win32_std_spec.cr @@ -221,6 +221,7 @@ require "./std/uint_spec.cr" require "./std/uri/punycode_spec.cr" require "./std/uri_spec.cr" require "./std/uuid_spec.cr" +# require "./std/va_list_spec.cr" require "./std/weak_ref_spec.cr" require "./std/xml/builder_spec.cr" require "./std/xml/html_spec.cr" diff --git a/src/compiler/crystal/semantic/main_visitor.cr b/src/compiler/crystal/semantic/main_visitor.cr index a4cff837c390..c6b4ec4009c4 100644 --- a/src/compiler/crystal/semantic/main_visitor.cr +++ b/src/compiler/crystal/semantic/main_visitor.cr @@ -2431,9 +2431,6 @@ module Crystal end def visit_va_arg(node) - if program.has_flag? "windows" - node.raise "va_arg is not yet supported on Windows" - end arg = call.not_nil!.args[0]? || node.raise("requires type argument") node.type = arg.type.instance_type end diff --git a/src/lib_c/aarch64-linux-gnu/c/stdarg.cr b/src/lib_c/aarch64-linux-gnu/c/stdarg.cr index 882d4f51d35c..965355556d71 100644 --- a/src/lib_c/aarch64-linux-gnu/c/stdarg.cr +++ b/src/lib_c/aarch64-linux-gnu/c/stdarg.cr @@ -1,3 +1,10 @@ lib LibC - type VaList = Void* + # based on https://github.com/llvm/llvm-project/blob/bf1cdc2c6c0460b7121ac653c796ef4995b1dfa9/clang/lib/AST/ASTContext.cpp#L7678-L7739 + struct VaList + __stack : Void* + __gr_top : Void* + __vr_top : Void* + __gr_offs : Int32 + __vr_offs : Int32 + end end diff --git a/src/lib_c/aarch64-linux-musl/c/stdarg.cr b/src/lib_c/aarch64-linux-musl/c/stdarg.cr index fcad7714f16a..965355556d71 100644 --- a/src/lib_c/aarch64-linux-musl/c/stdarg.cr +++ b/src/lib_c/aarch64-linux-musl/c/stdarg.cr @@ -1,10 +1,10 @@ lib LibC - struct VaListTag - gp_offset : UInt - fp_offset : UInt - overflow_arg_area : Void* - reg_save_area : Void* + # based on https://github.com/llvm/llvm-project/blob/bf1cdc2c6c0460b7121ac653c796ef4995b1dfa9/clang/lib/AST/ASTContext.cpp#L7678-L7739 + struct VaList + __stack : Void* + __gr_top : Void* + __vr_top : Void* + __gr_offs : Int32 + __vr_offs : Int32 end - - type VaList = VaListTag[1] end diff --git a/src/va_list.cr b/src/va_list.cr index eb0d0121f612..b24304c32e4c 100644 --- a/src/va_list.cr +++ b/src/va_list.cr @@ -18,7 +18,17 @@ struct VaList end end - {% if compare_versions(Crystal::VERSION, "0.33.0-0") > 0 %} + {% if flag?(:aarch64) || flag?(:win32) %} + {% platform = flag?(:aarch64) ? "AArch64" : "Windows" %} + {% clang_impl = flag?(:aarch64) ? "https://github.com/llvm/llvm-project/blob/a574edbba2b24fcfb733aa2d82308131f5b7d2d6/clang/lib/CodeGen/TargetInfo.cpp#L5677-L5921" : "https://github.com/llvm/llvm-project/blob/a574edbba2b24fcfb733aa2d82308131f5b7d2d6/clang/lib/CodeGen/TargetInfo.cpp#L5958-L5964" %} + # Do not call this, instead use C wrappers calling the va_arg macro for the types you need. + # + # Clang implements va_arg on {{platform.id}} like this: {{clang_impl.id}} + # If somebody wants to fix the LLVM IR va_arg instruction on {{platform}} upstream, or port the above here, that would be welcome. + def next(type) + \{% raise "Cannot get variadic argument on {{platform.id}}. As a workaround implement wrappers in C calling the va_arg macro for the types you need and bind to those." %} + end + {% else %} @[Primitive(:va_arg)] def next(type) end