Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix VaList and disable va_arg for AArch64 #9422

Merged
merged 1 commit into from
Jun 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions spec/compiler/codegen/primitives_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current state of this spec represents that it's not passing but really should be made to pass. This change drops that distinction.
(But I don't have an immediate suggestion how to keep that in a nice way, so feel free to disregard)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't immediately get why this was pending but the above was completely wrapped. I knew I wanted to wrap this one, so I unified to the majority.

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
6 changes: 1 addition & 5 deletions spec/spec_helper.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
11 changes: 11 additions & 0 deletions spec/std/spec_helper.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
40 changes: 40 additions & 0 deletions spec/std/va_list_spec.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
require "./spec_helper"

describe VaList do
it "works with C code" do
jhass marked this conversation as resolved.
Show resolved Hide resolved
compile_and_run_source_with_c(
%(
#include <stdarg.h>
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
10 changes: 10 additions & 0 deletions spec/support/tempfile.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions spec/win32_std_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
3 changes: 0 additions & 3 deletions src/compiler/crystal/semantic/main_visitor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion src/lib_c/aarch64-linux-gnu/c/stdarg.cr
Original file line number Diff line number Diff line change
@@ -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
14 changes: 7 additions & 7 deletions src/lib_c/aarch64-linux-musl/c/stdarg.cr
Original file line number Diff line number Diff line change
@@ -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
12 changes: 11 additions & 1 deletion src/va_list.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down