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

Add VaList API #5103

Merged
merged 2 commits into from Jan 4, 2018
Merged

Add VaList API #5103

merged 2 commits into from Jan 4, 2018

Conversation

makenowjust
Copy link
Contributor

Now we can compile and run this code:

lib LibC
  fun vsnprintf(buf : Char*, n : SizeT, fmt : Char*, arg : VaList) : Int
end

{% `
cat <<C >foo.c
int foo(int(*f)()) {
  return f(1, 2, 3);
}
C
gcc -c -o foo.o foo.c
` %}
@[Link(ldflags: "#{__DIR__}/foo.o")]
lib Foo
  fun foo(f : -> LibC::Int) : LibC::Int
end

# https://github.com/crystal-lang/crystal/pull/5102
#
# proc_f = ->(...) do
#   VaList.open do |list|
#     buf = uninitialized UInt8[128]
#     result = LibC.vsnprintf buf, 127, "proc_f(%d, %d, %d)", list
#     puts String.new(buf.to_unsafe)
#     result
#   end
# end

fun fun_f(...) : Int32
  VaList.open do |list|
    buf = uninitialized UInt8[128]
    result = LibC.vsnprintf buf, 127, "fun_f(%d, %d, %d)", list
    puts String.new(buf.to_unsafe)
    result
  end
end

`rm -rf foo.o foo.c`

# Foo.foo proc_f
Foo.foo ->fun_f

Added API is VaList.open only. VaList.open needs a block and it passes VaList pointer to this block. It is combination of va_start and va_end. And this PR doesn't provide va_arg like method because I think it is not needed for Crystal.


NOTE: aarch64, arm and freebsd and openbsd are not tested. Because I don't know how to run Crystal on these environment (and I don't have aarch64 and arm machine.) And specs for VaList are missing. How do I write this?

@ysbaddaden Please help me!

`aarch64`, `arm` and `freebsd` and `openbsd` are not tested. Because I
don't know how to run Crystal on these environment (and I don't have
aarch64 and arm machine.)
@Papierkorb
Copy link
Contributor

Wouldn't it be possible to have the VaList be a type used as argument like fun sum(count : Int32, ints : VaList), and the compiler would code-gen the necessary va_* by itself? That way, you could name the argument list, and work with it almost like with an Iterator.

@makenowjust
Copy link
Contributor Author

@Papierkorb It is impossible currently because VaList has no method like va_arg so we cannot get a value from VaList instance. To implement va_arg like method is easy but I don't know when this method is used. Do you know the C library using complex variadic function pointer?

@Papierkorb
Copy link
Contributor

@makenowjust Do you mean if I know a C library using a function pointer to a variadic function, or if I know how to use va_list? Can't tell you a C lib, but I've used va_list in my C/C++ past ;)

@makenowjust
Copy link
Contributor Author

@Papierkorb I said about C lib.

@Papierkorb
Copy link
Contributor

Papierkorb commented Oct 11, 2017

But as you mentioned, implementing a va_arg pendant is non trivial. If I remember correctly, the C standard says that an "entry" in a va list is the size of void*. So in the first step, it could return a Void*, and a wrapper function built ontop of that could do casting in normal, non-magic Crystal code like:

struct VaList
  def next : Void* # Magic

  def next_i32 : Int32 # And conversion functions for convenience
    next.value.to_i32
  end
end

However I may be overlooking something.

Edit: I don't know of a C library right of the bat.

@makenowjust
Copy link
Contributor Author

makenowjust commented Oct 11, 2017

@Papierkorb It is not so simple on x86-64 call convention. See https://github.com/MakeNowJust/crystal/blob/13cc38fb2f17b660f1a04333969f78df351f38e5/src/lib_c/x86_64-macosx-darwin/c/stdarg.cr for example. We should use LLVM intrinsics to use va_list.

@Papierkorb
Copy link
Contributor

We should use llvm API to use va_list.

Absolutely, otherwise this sounds like a nightmare to support for all ABIs.

@ysbaddaden
Copy link
Contributor

I was about to say that this could simplify JNI bindings but no: supporting ... variadic arguments in procs would.

We should use llvm API to use va_list.

Yes.

@ysbaddaden
Copy link
Contributor

And I totally misunderstood, and further don't understand much the need. Do you have examples of C libraries that callback to Crystal with variadic arguments?

@makenowjust
Copy link
Contributor Author

makenowjust commented Oct 11, 2017

@ysbaddaden For example in stdlib,

# TODO: use va_start and va_end to

@makenowjust
Copy link
Contributor Author

I want to resolve this TODO by this PR and #5102. It is only my motivation.


def self.open
ap = uninitialized LibC::VaList
Intrinsics.va_start pointerof(ap)
Copy link
Member

Choose a reason for hiding this comment

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

Can you use out ap here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No because va_start needs Void* but ap must be VaList.

Copy link
Member

Choose a reason for hiding this comment

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

I see, makes sense.

src/va_list.cr Outdated
@@ -0,0 +1,20 @@
require "c/stdarg"

class VaList
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.

@bew
Copy link
Contributor

bew commented Oct 13, 2017

Do you plan to add va_copy ?

@makenowjust
Copy link
Contributor Author

makenowjust commented Oct 14, 2017

@bew va_copy support sounds good but I have no idea to coerce to call va_end against the copy.

@Papierkorb
Copy link
Contributor

Make the #dup, which would do the copy, return a new VaList.

@makenowjust
Copy link
Contributor Author

Perhaps #clone is more preferred naming in Crystal because #clone means deep copy and #dup means shallow copy. However it is not point for current discussion, I'm considering how to coerce to call va_end against all VaList pointers.

@asterite
Copy link
Member

@makenowjust This is excellent, thank you!

Do you think you can fix the TODO here?

crystal/src/xml/error.cr

Lines 22 to 34 in 2637f83

LibXML.xmlSetGenericErrorFunc nil, ->(ctx, fmt) {
# TODO: use va_start and va_end to
message = String.new(fmt).chomp
error = XML::Error.new(message, 0)
{% if flag?(:arm) || flag?(:aarch64) %}
# libxml2 is likely missing ARM unwind tables (.ARM.extab and .ARM.exidx
# sections) which prevent raising from a libxml2 context.
@@errors << error
{% else %}
raise error
{% end %}
}

That way we can see whether VaList is correctly implemented and useful.

I think you can try that out by parsing an XML with errors, the error has a format and arguments that are usable with C's printf.

But this is not strictly necessary in this PR, so you can leave it for later, or for someone else to implement it. But without using VaList it's a bit hard to know if it's implemented correctly.

@makenowjust
Copy link
Contributor Author

@asterite I think it is better too. I'll try to fix this.

But I repeat:

aarch64, arm, freebsd and openbsd are not tested.

To fix this is more important, but it is hard for me to prepare such a os/architecture environment to test. Please help me.

@bew
Copy link
Contributor

bew commented Oct 21, 2017

Will it fix/close #214 also?

@makenowjust
Copy link
Contributor Author

No

@sdogruyol
Copy link
Member

Is there anyone else who can review this 😿

@RX14 RX14 merged commit 501e3ba into crystal-lang:master Jan 4, 2018
@RX14
Copy link
Contributor

RX14 commented Jan 4, 2018

Just for future reference: https://llvm.org/docs/LangRef.html#va-arg-instruction

va_arg is actually implemented by LLVM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants