search

kwilczynski / ruby-magic

Simple interface to libmagic for Ruby Programming Language
Apache License 2.0
27 stars 8 forks source link

Add Truffleruby head to CI #10

Closed gogainda closed 3 years ago

kwilczynski commented 3 years ago

Hi @gogainda, thank you for this!

I have a question - does this Ruby Gem actually builds and works correctly with TruffleRuby? I don't have any experience using it, especially not with C extensions. Do you have some experience with it @stanhu?

Did you have things running OK for you? If so, that's great, but also we should wait for the https://github.com/kwilczynski/ruby-magic/pull/5 to be completed and merged, and then see if everything is still working.

Krzysztof

gogainda commented 3 years ago

ok, doesn't look like it's working on truffleruby, will close for now

kwilczynski commented 3 years ago

Hi @gogainda, thank you for checking!

How does it "not work" - does it failed to build? Not loading? Crashing?

I would imagine that TruffleRuby would have the same problems with this extension as JRuby.

Krzysztof

stanhu commented 3 years ago

@kwilczynski Strange, Truffleruby seems to be hitting an error just compiling the C extension (https://travis-ci.org/github/kwilczynski/ruby-magic/jobs/764661248):

checking for ruby.h... Error while formatting Ruby exception:
org.truffleruby.core.rope.CannotConvertBinaryRubyStringToJavaString: Cannot convert a Ruby String with BINARY encoding containing non-US-ASCII character 226 to a Java String
    at org.truffleruby.core.rope.RopeOperations.decodeNonAscii(RopeOperations.java:184)
    at org.truffleruby.core.rope.RopeOperations.decodeRopeSegment(RopeOperations.java:241)
    at org.truffleruby.core.rope.RopeOperations.decodeRopeSegment(RopeOperations.java:234)
    at org.truffleruby.core.rope.RopeOperations.decodeRope(RopeOperations.java:229)
    at org.truffleruby.core.string.RubyString.getJavaString(RubyString.java:63)
    at org.truffleruby.core.string.RubyStringGen$RubyStringLibraryExports$Uncached.getJavaString(RubyStringGen.java:331)
    at org.truffleruby.language.library.RubyStringLibraryGen$UncachedDispatch.getJavaString(RubyStringLibraryGen.java:404)
    at org.truffleruby.language.backtrace.BacktraceFormatter.printRubyExceptionOnEnvStderr(BacktraceFormatter.java:124)
    at org.truffleruby.language.backtrace.BacktraceFormatter.printTopLevelRubyExceptionOnEnvStderr(BacktraceFormatter.java:106)
    at org.truffleruby.language.exceptions.TopLevelRaiseHandler.execute(TopLevelRaiseHandler.java:63)
    at org.truffleruby.language.TruffleBootNodes$MainNode.main(TruffleBootNodes.java:118)
    at org.truffleruby.language.TruffleBootNodesFactory$MainNodeFactory$MainNodeGen.executeAndSpecialize(TruffleBootNodesFactory.java:362)
    at org.truffleruby.language.TruffleBootNodesFactory$MainNodeFactory$MainNodeGen.execute(TruffleBootNodesFactory.java:343)
    at org.truffleruby.language.arguments.CheckArityNode.execute(CheckArityNode.java:45)
    at org.truffleruby.language.methods.ExceptionTranslatingNode.execute(ExceptionTranslatingNode.java:33)
    at org.truffleruby.language.RubyRootNode.execute(RubyRootNode.java:61)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.executeRootNode(OptimizedCallTarget.java:613)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.profiledPERoot(OptimizedCallTarget.java:584)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callBoundary(OptimizedCallTarget.java:534)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTarget.invokeCallBoundary(SubstrateOptimizedCallTarget.java:121)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTargetInstalledCode.doInvoke(SubstrateOptimizedCallTargetInstalledCode.java:164)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTarget.doInvoke(SubstrateOptimizedCallTarget.java:104)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callDirect(OptimizedCallTarget.java:481)
    at org.graalvm.compiler.truffle.runtime.OptimizedDirectCallNode.call(OptimizedDirectCallNode.java:71)
    at org.truffleruby.language.methods.CallInternalMethodNode.callCached(CallInternalMethodNode.java:57)
    at org.truffleruby.language.methods.CallInternalMethodNodeGen.executeAndSpecialize(CallInternalMethodNodeGen.java:145)
    at org.truffleruby.language.methods.CallInternalMethodNodeGen.execute(CallInternalMethodNodeGen.java:92)
    at org.truffleruby.language.dispatch.DispatchNode.dispatch(DispatchNode.java:138)
    at org.truffleruby.language.dispatch.RubyCallNode.executeWithArgumentsEvaluated(RubyCallNode.java:137)
    at org.truffleruby.language.dispatch.RubyCallNode.execute(RubyCallNode.java:98)
    at org.truffleruby.language.methods.ExceptionTranslatingNode.execute(ExceptionTranslatingNode.java:33)
    at org.truffleruby.language.control.SequenceNode.execute(SequenceNode.java:36)
    at org.truffleruby.language.methods.CatchForLambdaNode.execute(CatchForLambdaNode.java:53)
    at org.truffleruby.language.RubyRootNode.execute(RubyRootNode.java:61)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.executeRootNode(OptimizedCallTarget.java:613)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.profiledPERoot(OptimizedCallTarget.java:584)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callBoundary(OptimizedCallTarget.java:534)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTarget.invokeCallBoundary(SubstrateOptimizedCallTarget.java:121)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTargetInstalledCode.doInvoke(SubstrateOptimizedCallTargetInstalledCode.java:164)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTarget.doInvoke(SubstrateOptimizedCallTarget.java:104)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callDirect(OptimizedCallTarget.java:481)
    at org.graalvm.compiler.truffle.runtime.OptimizedDirectCallNode.call(OptimizedDirectCallNode.java:71)
    at org.truffleruby.language.yield.CallBlockNode.callBlockCached(CallBlockNode.java:61)
    at org.truffleruby.language.yield.CallBlockNodeGen.executeAndSpecialize(CallBlockNodeGen.java:106)
    at org.truffleruby.language.yield.CallBlockNodeGen.executeCallBlock(CallBlockNodeGen.java:56)
    at org.truffleruby.language.yield.CallBlockNode.yield(CallBlockNode.java:44)
    at org.truffleruby.core.proc.RubyProc.execute(RubyProc.java:137)
    at org.truffleruby.core.proc.RubyProcGen$InteropLibraryExports$Cached.executeAndSpecialize(RubyProcGen.java:118)
    at org.truffleruby.core.proc.RubyProcGen$InteropLibraryExports$Cached.execute(RubyProcGen.java:104)
    at com.oracle.truffle.api.interop.InteropLibraryGen$CachedDispatch.execute(InteropLibraryGen.java:7478)
    at com.oracle.truffle.polyglot.PolyglotValue$InteropCodeCache$AbstractExecuteNode.executeShared(PolyglotValue.java:3079)
    at com.oracle.truffle.polyglot.PolyglotValue$InteropCodeCache$ExecuteNode.executeImpl(PolyglotValue.java:3160)
    at com.oracle.truffle.polyglot.HostToGuestRootNode.execute(HostToGuestRootNode.java:134)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.executeRootNode(OptimizedCallTarget.java:613)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.profiledPERoot(OptimizedCallTarget.java:584)
    at org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callBoundary(OptimizedCallTarget.java:534)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTarget.invokeCallBoundary(SubstrateOptimizedCallTarget.java:121)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTargetInstalledCode.doInvoke(SubstrateOptimizedCallTargetInstalledCode.java:164)
    at com.oracle.svm.truffle.api.SubstrateOptimizedCallTarget.doInvoke(SubstrateOptimizedCallTarget.java:104)
    at org.graalvm.compiler.truffle.runtime.GraalRuntimeSupport.callProfiled(GraalRuntimeSupport.java:143)
    at com.oracle.truffle.polyglot.PolyglotValue$InteropValue.execute(PolyglotValue.java:4519)
    at org.graalvm.polyglot.Value.execute(Value.java:832)
    at org.truffleruby.launcher.RubyLauncher.runRubyMain(RubyLauncher.java:227)
    at org.truffleruby.launcher.RubyLauncher.launch(RubyLauncher.java:123)
    at org.graalvm.launcher.AbstractLanguageLauncher.launch(AbstractLanguageLauncher.java:124)
    at org.graalvm.launcher.AbstractLanguageLauncher.launch(AbstractLanguageLauncher.java:71)
    at org.truffleruby.launcher.RubyLauncher.main(RubyLauncher.java:38)
Original Ruby exception:
(exception org.truffleruby.core.rope.CannotConvertBinaryRubyStringToJavaString Cannot convert a Ruby String with BINARY encoding containing non-US-ASCII character 226 to a Java String org.truffleruby.core.rope.RopeOperations.decodeNonAscii(RopeOperations.java:184)
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:688:in `try_cpp'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:1199:in `block in have_header'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:1046:in `block in checking_for'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:429:in `block (2 levels) in postpone'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:390:in `open'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:429:in `block in postpone'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:390:in `open'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:425:in `postpone'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:1045:in `checking_for'
    from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:1198:in `have_header'
    from ../../../../ext/magic/extconf.rb:81:in `<main>'

*** ../../../../ext/magic/extconf.rb failed ***
Could not create Makefile due to some reason, probably lack of necessary

It fails on building the simplest Ruby program:

/* begin */
1: #include "ruby.h"
2: 
3: int main(int argc, char **argv)
4: {
5:   return !!argv[argc];
6: }
/* end */

It doesn't look like this is specific to ruby-magic, though.

gogainda commented 3 years ago

Interestingly, it passed the compilation stage on my Mac, but failed during test run: compilation - https://gist.github.com/gogainda/9c599346ed4699f4752d2e6b5395db7f test run - https://gist.github.com/gogainda/ca5a02366e95f73db0ac85167ef7c824

We may ask @eregon to hear his opinion

kwilczynski commented 3 years ago

Hi @gogainda and @stanhu, thank you both for looking into this!

@kwilczynski Strange, Truffleruby seems to be hitting an error just compiling the C extension (travis-ci.org/github/kwilczynski/ruby-magic/jobs/764661248): (...)

Original Ruby exception:
(exception org.truffleruby.core.rope.CannotConvertBinaryRubyStringToJavaString Cannot convert a Ruby String with BINARY encoding containing non-US-ASCII character 226 to a Java String org.truffleruby.core.rope.RopeOperations.decodeNonAscii(RopeOperations.java:184)
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:688:in `try_cpp'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:1199:in `block in have_header'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:1046:in `block in checking_for'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:429:in `block (2 levels) in postpone'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:390:in `open'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:429:in `block in postpone'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:390:in `open'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:425:in `postpone'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:1045:in `checking_for'
  from /home/travis/.rvm/rubies/truffleruby-head/lib/mri/mkmf.rb:1198:in `have_header'
  from ../../../../ext/magic/extconf.rb:81:in `<main>'

(...)

The character that allegedly breaks the compilation:

[1] pry(main)> 226.chr(Encoding::UTF_8)
"â"
[2] pry(main)> 226.chr(Encoding::ASCII_8BIT)
"\xE2"
[3] pry(main)>

[...]

It fails on building the simplest Ruby program:

/* begin */
1: #include "ruby.h"
2: 
3: int main(int argc, char **argv)
4: {
5:   return !!argv[argc];
6: }
/* end */

It doesn't look like this is specific to ruby-magic, though.

Something is also not passing -rpath as an option correctly, as per:

"/usr/bin/gcc -o conftest -I/home/travis/.rvm/rubies/truffleruby-head/lib/cext/include -I/home/travis/.rvm/rubies/truffleruby-head/lib/cext/include/ruby/backward -I/home/travis/.rvm/rubies/truffleruby-head/lib/cext/include -I../../../../ext/magic      -Wimplicit-function-declaration -Wno-int-conversion -Wno-int-to-pointer-cast -Wno-incompatible-pointer-types -Wno-format-invalid-specifier -Wno-format-extra-args -ferror-limit=500 -I /home/travis/build/kwilczynski/ruby-magic/tmp/x86_64-linux/magic/2.7.2/file-5.39/src -std=c99 -fPIC -Wall -Wextra -pedantic -O3 -Wcast-qual -Wwrite-strings -Wconversion -Wmissing-noreturn -Winline -I/include conftest.c  -L. -L/home/travis/build/kwilczynski/ruby-magic/lib/ext/lib -Wl,-rpath,/home/travis/build/kwilczynski/ruby-magic/lib/ext/lib  -Wl,--as-needed -Wl,--no-undefined -L/lib   -lmagic  -lgraalvm-llvm    -L/home/travis/.rvm/rubies/truffleruby-head/lib/cext -rpath /home/travis/.rvm/rubies/truffleruby-head/lib/cext -ltruffleruby -rpath /home/travis/.rvm/rubies/truffleruby-head/lib/sulong/native/lib"
gcc: error: unrecognized command line option ‘-ferror-limit=500’
gcc: error: unrecognized command line option ‘-rpath’
gcc: error: unrecognized command line option ‘-rpath’

I wonder if this is causing this simple main.c fail to compile. Unless GCC just treats this error as non-fatal, so to speak.

Krzysztof

kwilczynski commented 3 years ago

Hi @gogainda, this is great!

Interestingly, it passed the compilation stage on my Mac, but failed during test run: compilation - https://gist.github.com/gogainda/9c599346ed4699f4752d2e6b5395db7f test run - https://gist.github.com/gogainda/ca5a02366e95f73db0ac85167ef7c824

The problem lies with calling the lock method on the embedded Mutex object. Perhaps TruffleRuby's implementation of the Mutex object is slightly different to the one in the vanilla Ruby.

Would you be able to give the following patch a try?

diff --git i/ext/magic/ruby-magic.c w/ext/magic/ruby-magic.c
index 91a1d74..93460cc 100644
--- i/ext/magic/ruby-magic.c
+++ w/ext/magic/ruby-magic.c
@@ -1268,7 +1268,7 @@ magic_lock(VALUE object, VALUE(*function)(ANYARGS), void *data)
    magic_object_t *mo;

    MAGIC_OBJECT(mo);
-   rb_funcall(mo->mutex, rb_intern("lock"), 0, Qundef);
+   rb_funcall(mo->mutex, rb_intern("lock"), 0);

    return rb_ensure(function, (VALUE)data, magic_unlock, object);
 }
@@ -1279,7 +1279,7 @@ magic_unlock(VALUE object)
    magic_object_t *mo;

    MAGIC_OBJECT(mo);
-   rb_funcall(mo->mutex, rb_intern("unlock"), 0, Qundef);
+   rb_funcall(mo->mutex, rb_intern("unlock"), 0);

    return Qnil;
 }
diff --git i/ext/magic/ruby-magic.h w/ext/magic/ruby-magic.h
index 413e983..37f756f 100644
--- i/ext/magic/ruby-magic.h
+++ w/ext/magic/ruby-magic.h
@@ -156,8 +156,8 @@ static const char *errors[] = {
 static inline VALUE
 magic_shift(VALUE v)
 {
-   return ARRAY_P(v) ?                    \
-       rb_funcall(v, rb_intern("shift"), 0, Qundef) : \
+   return ARRAY_P(v) ?                \
+       rb_funcall(v, rb_intern("shift"), 0) : \
        Qnil;
 }

@@ -180,8 +180,8 @@ magic_join(VALUE a, VALUE b)
 static inline VALUE
 magic_flatten(VALUE v)
 {
-   return ARRAY_P(v) ?                  \
-       rb_funcall(v, rb_intern("flatten"), 0, Qundef) : \
+   return ARRAY_P(v) ?              \
+       rb_funcall(v, rb_intern("flatten"), 0) : \
        Qnil;
 }

@@ -192,7 +192,7 @@ magic_fileno(VALUE object)
    rb_io_t *io;

    if (rb_respond_to(object, rb_intern("fileno"))) {
-       object = rb_funcall(object, rb_intern("fileno"), 0, Qundef);
+       object = rb_funcall(object, rb_intern("fileno"), 0);
        return NUM2INT(object);
    }

@@ -213,13 +213,13 @@ magic_path(VALUE object)
        return object;

    if (rb_respond_to(object, rb_intern("to_path")))
-       return rb_funcall(object, rb_intern("to_path"), 0, Qundef);
+       return rb_funcall(object, rb_intern("to_path"), 0);

    if (rb_respond_to(object, rb_intern("path")))
-       return rb_funcall(object, rb_intern("path"), 0, Qundef);
+       return rb_funcall(object, rb_intern("path"), 0);

    if (rb_respond_to(object, rb_intern("to_s")))
-       return rb_funcall(object, rb_intern("to_s"), 0, Qundef);
+       return rb_funcall(object, rb_intern("to_s"), 0);

    return Qnil;
 }

This patch is to be applied to the state of the codebase at the following commit: https://github.com/kwilczynski/ruby-magic/commit/3355199c5dce18e596a21f545e3141a07aa4d9c8 - this will point you are the state before we started to vendor libmagic alongside.

Krzysztof

kwilczynski commented 3 years ago

Hi @gogainda,

I am happy to report, that with the above patch, the extension does work fine - it compiles, tests and works without issues.

However, there are some subtle differences in the API between vanilla Ruby and TruffleRuby:

<internal:core> core/throw_catch.rb:36:in `catch'
     685:   def test_magic_singleton_do_not_auto_load_global
     686:     assert_false(Magic.do_not_auto_load)
     687:
  => 688:     fork do
     689:       Magic.do_not_auto_load = true
     690:
     691:       magic_1 = Magic.new
/Users/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/test/test_magic.rb:688:in `test_magic_singleton_do_not_auto_load_global'
<internal:core> core/kernel.rb:758:in `fork'
Error: test_magic_singleton_do_not_auto_load_global(MagicTest): NotImplementedError: fork is not available
<internal:core> core/throw_catch.rb:36:in `catch'
     486:       @magic.buffer nil
     487:     end
     488:
  => 489:     assert_equal('wrong argument type nil (expected String)', error.message)
     490:   end
     491:
     492:   def test_magic_buffer_argument_with_NULL
/Users/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/test/test_magic.rb:489:in `test_magic_buffer_with_nil_argument'
<"wrong argument type nil (expected String)"> expected but was
<"wrong argument type NilClass (expected 5)">

diff:
? wrong argument type nil      (expected String)
?                     N  Class           5
Failure: test_magic_buffer_with_nil_argument(MagicTest)

But this is all most likely nothing that can't be fixed.

Krzysztof

gogainda commented 3 years ago

@kwilczynski feel free to create an issue here if think it should be fixed on Truffleruby side

kwilczynski commented 3 years ago

Hi @gogainda,

I could, but I am not sure if @eregon would have an interest in trying to address some discrepancies.

Perhaps I can address some of the issues on my side. How urgently do you need to have TruffleRuby support @gogainda?

Krzysztof

gogainda commented 3 years ago

I am not in a hurry, but it's better to log the issues for the future development

eregon commented 3 years ago

We're happy to fix error messages differences, could you file a short issue about that ?

The diff in https://github.com/kwilczynski/ruby-magic/pull/10#issuecomment-809816410 looks good, those Qundef should indeed not be there AFAIK.

Regarding the non-ASCII character â, did you find where it comes from?

kwilczynski commented 3 years ago

Hi @eregon, thank you for getting back to me!

We're happy to fix error messages differences, could you file a short issue about that ?

The error message difference is interesting, or rather, the reason why we are seeing it. This is mainly related to subtle differences between Ruby and TruffleRuby (or even JRuby where TruffleRuby has its origins).

I am using function and a macro from the Ruby C API to tell me what the type and also name of the object is when printing an error, then Ruby returns nil as a type, not class, of the object that has been passed, and then correctly resolves internal identifier of an object type to its name (e.g., String, etc.).

TruffleRuby (and JRuby, as seen in the error messages people posted here and there) returns here class name for the type of the object, so NilClass, which is not correct. It also does not seem to know how to resolve a name for based on the internal type ID, and simple prints the value of 5 (see: include/ruby/internal/value_type.h#L117).

The nil type vs NilClass is probably something that JRuby and TruffleRuby could fix, if desired. What do you think @eregon?

Having said that, I could probably add small wrappers to normalise things in my code so that the correct value would be printed regardless of the underlying platform.

The diff in #10 (comment) looks good, those Qundef should indeed not be there AFAIK.

Ruby's rb_funcall() is different to TruffleRuby's. With Ruby version, if the number of arguments is to be 0, then it would not try to build a variable arguments list passed to it - and the last argument in the method signature is a variable length argument.

Sadly, if you don't pass anything, then compiler will complain, for example:

../../../../ext/magic/ruby-magic.c:1282:46: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]

Thus, people have been customary passing 0. You can't really pass NULL and on a lot of platforms that expands to void * which would raise even more warnings since variable arguments are of a VALUE type which is const unsigned long. I passed Qundef which is an internal type and would in most of the cases guarantee a crash (the desired behaviour) where the arguments should not be consumed (passing Qnil could lead to an exception or a silent failure since it has special meaning).

Now, for some reason TruffleRuby's version of rb_funcall() seem to be always passing arguments, regardless whether 0 is provided as argument count. Both implementations as follows to illustrate the difference.

VALUE
rb_funcall(VALUE recv, ID mid, int n, ...)
{
    VALUE *argv;
    va_list ar;

    if (n > 0) {
    long i;

        va_start(ar, n);

    argv = ALLOCA_N(VALUE, n);

    for (i = 0; i < n; i++) {
        argv[i] = va_arg(ar, VALUE);
    }
    va_end(ar);
    }
    else {
    argv = 0;
    }
    return rb_funcallv(recv, mid, n, argv);
}
  def rb_funcall(recv, meth, n, *args)
    # see #call_with_thread_locally_stored_block
    thread_local_block = Thread.current[:__C_BLOCK__]
    Thread.current[:__C_BLOCK__] = nil
    recv.__send__(meth, *args, &thread_local_block)
  ensure
    Thread.current[:__C_BLOCK__] = thread_local_block
  end

We can see in the above that n is not used for anything and arguments are passed as-is always. I would consider this a bug, and I am happy to open an issue to report this, if you want @eregon.

Regarding the non-ASCII character â, did you find where it comes from?

No idea. I believe it's a bug (at least such is my hypothesis). We don't use non-ASCII characters in any of the files used in the project. I also doubt that mkmf would create anything with random UTF-8 characters in it either.

Krzysztof

eregon commented 3 years ago

The nil type vs NilClass is probably something that JRuby and TruffleRuby could fix, if desired. What do you think @eregon?

Yes, that's worth reporting. If you can tell which C API function does that it would be helpful, then we should be able to easily write a test for it. nil/true/false are typically treated specially in error messages, so we might have missed this specific case.

../../../../ext/magic/ruby-magic.c:1282:46: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]

I would recommend to just disable this warning (-Wno-gnu-zero-variadic-macro-arguments). It's a well known extension that about every C compiler supports, because it's simply needed for C code. I think (but not sure) that this warning also doesn't happen on more recent standards like C99+, so it's more of a relic of the past by now. https://github.com/ruby/ruby/blob/68d028578a5975fe6c44d29c502615ff9af72927/enumerator.c#L1363 and C API specs for instance just do rb_funcall(method, idTo_proc, 0); and I've never seen any warning for it.

You can also use rb_funcallv(recv, someID, 0, NULL), that should be fully valid in all cases.

What TruffleRuby does is it uses how many arguments are passed to rb_funcall to choose the right macro: https://github.com/oracle/truffleruby/blob/e9de97bd442a15dbff0bc01796fae0cf2ab29416/lib/cext/include/truffleruby/truffleruby.h#L163 That's not gonna be easy to change, so I think we need the change in the gem in this case.

eregon commented 3 years ago

Actually that warning is probably only in old Ruby versions, for instance the dev version defines rb_funcall as https://github.com/ruby/ruby/blob/1f08b0d18d9d1178cef1eac91f6ea97cb6b437eb/include/ruby/ruby.h#L126-L129 Maybe that __extension__ silences the warning automatically, it's just a guess though.

kwilczynski commented 3 years ago

Hi @eregon,

Actually that warning is probably only in old Ruby versions, for instance the dev version defines rb_funcall as https://github.com/ruby/ruby/blob/1f08b0d18d9d1178cef1eac91f6ea97cb6b437eb/include/ruby/ruby.h#L126-L129

The following is what I currently have in my runtime environment:

   kwilczynski@workstation  Ruby/ruby-magic   main ✘  $ gcc --version
gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include/c++/4.2.1
Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Ruby version:

   kwilczynski@workstation  Ruby/ruby-magic   main ✘  $ ruby -v
ruby 2.7.2p137 (2020-10-01 revision 5445e04352) [x86_64-linux]

Maybe that __extension__ silences the warning automatically, it's just a guess though.

Ruby version 2.7.2 has it added (see: include/ruby/ruby.h#L2683-L2693). Sadly it does not seem to hint the compiler and the warning is still present.

Krzysztof

kwilczynski commented 3 years ago

Hi @eregon,

The nil type vs NilClass is probably something that JRuby and TruffleRuby could fix, if desired. What do you think @eregon?

Yes, that's worth reporting. If you can tell which C API function does that it would be helpful, then we should be able to easily write a test for it. nil/true/false are typically treated specially in error messages, so we might have missed this specific case.

OK. I will have a look and report back. Thank you!

[...]

I think (but not sure) that this warning also doesn't happen on more recent standards like C99+, so it's more of a relic of the past by now.

We are building with -std=c99, and both Clang and GCC would complain by default.

https://github.com/ruby/ruby/blob/68d028578a5975fe6c44d29c502615ff9af72927/enumerator.c#L1363 and C API specs for instance just do rb_funcall(method, idTo_proc, 0); and I've never seen any warning for it.

Very true. I have never seen this problem being reported when building Ruby. I have an idea, though, what enables the compiler option which is responsible for the warning. Stay put. I will report shortly.

You can also use rb_funcallv(recv, someID, 0, NULL), that should be fully valid in all cases.

That would work! Good idea! Thank you :)

What TruffleRuby does is it uses how many arguments are passed to rb_funcall to choose the right macro: https://github.com/oracle/truffleruby/blob/e9de97bd442a15dbff0bc01796fae0cf2ab29416/lib/cext/include/truffleruby/truffleruby.h#L163 That's not gonna be easy to change, so I think we need the change in the gem in this case.

Oh I see. Hmm. But, do you consider this an issue worth solving?

Krzysztof

eregon commented 3 years ago

But, do you consider this an issue worth solving?

If it appears in many gems TruffleRuby might need to for compatibility, but I think so far we've never encountered the issue (before this) and yet many C extensions run on TruffleRuby already. Maybe it could be done by special casing the macro with argument and check if argc is 0, but it feels like a hack. I think we'd rather raise a proper error if the number of passed arguments and argc differ.

kwilczynski commented 3 years ago

Hi @eregon,

[...]

I think (but not sure) that this warning also doesn't happen on more recent standards like C99+, so it's more of a relic of the past by now.

We are building with -std=c99, and both Clang and GCC would complain by default.

https://github.com/ruby/ruby/blob/68d028578a5975fe6c44d29c502615ff9af72927/enumerator.c#L1363 and C API specs for instance just do rb_funcall(method, idTo_proc, 0); and I've never seen any warning for it.

Very true. I have never seen this problem being reported when building Ruby. I have an idea, though, what enables the compiler option which is responsible for the warning. Stay put. I will report shortly. [...]

I have refactored the way how custom CFLAGS et al are set, removed some surplus flags that Ruby was already setting, and also removed the -pedantic as we don't really needed to strictly enforce the compliance with the standard at the cost of disabling some extensions and enabling more warnings. There would be no problem now with the variadic function argument not being passed.

Krzysztof

kwilczynski commented 3 years ago

Hi @eregon, sorry for late reply!

[...]

The nil type vs NilClass is probably something that JRuby and TruffleRuby could fix, if desired. What do you think @eregon?

Yes, that's worth reporting. If you can tell which C API function does that it would be helpful, then we should be able to easily write a test for it. nil/true/false are typically treated specially in error messages, so we might have missed this specific case.

OK. I will have a look and report back. Thank you! [...]

I was able to derive a small piece of code that can reproduce what I was talking about:

#if defined(__cplusplus)
extern "C" {
#endif

#include <ruby.h>

void
Init_test(void)
{
    printf("%s\n", rb_class2name(rb_cNilClass));
    printf("%s\n", rb_obj_classname(Qnil));

    printf("%s\n", rb_class2name(rb_cString));
    printf("%s\n", rb_obj_classname(rb_str_new2("")));

    /* Check_Type(Qnil, T_STRING); */
    /* Check_Type(INT2NUM(0), T_STRING); */
    rb_check_type(Qnil, T_STRING);
}

#if defined(__cplusplus)
}
#endif
# frozen_string_literal: true

require 'mkmf'

dir_config('test')

create_header
create_makefile('test')

Running it against Ruby and TruffleRuby:

   kwilczynski@rocinante  Ruby/test  $ rbenv local 3.0.0
   kwilczynski@rocinante  Ruby/test  $ ruby -v
ruby 3.0.0p0 (2020-12-25 revision 95aff21468) [x86_64-darwin19]
   kwilczynski@rocinante  Ruby/test  $ ruby extconf.rb
creating extconf.h
creating Makefile
   kwilczynski@rocinante  Ruby/test  $ make
compiling test.c
linking shared-object test/test.bundle
   kwilczynski@rocinante  Ruby/test  $ RUBYLIB=. ruby -rtest -e ''
NilClass
NilClass
String
String
<internal:/Users/kwilczynski/.rbenv/versions/3.0.0/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:85:in `require': wrong argument type nil (expected String) (TypeError)
    from <internal:/Users/kwilczynski/.rbenv/versions/3.0.0/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
   kwilczynski@rocinante  Ruby/test  $ rbenv local truffleruby-21.0.0
   kwilczynski@rocinante  Ruby/test  $ ruby -v
truffleruby 21.0.0, like ruby 2.7.2, GraalVM CE Native [x86_64-darwin]
   kwilczynski@rocinante  Ruby/test  $ ruby extconf.rb
creating extconf.h
creating Makefile
   kwilczynski@rocinante  Ruby/test  $ make
compiling test.c
linking shared-object test/test.bundle
   kwilczynski@rocinante  Ruby/test  $ RUBYLIB=. ruby -rtest -e ''
NilClass
NilClass
String
String
/Users/kwilczynski/.rbenv/versions/truffleruby-21.0.0/lib/truffle/truffle/cext.rb:208:in `rb_check_type': wrong argument type NilClass (expected 5) (TypeError)
    from object.c:35:in `rb_check_type'
    from /Users/kwilczynski/Development/Projects/Personal/Ruby/test/test.c:16:in `Init_test'

The function I am using is Check_Type() from include/ruby/internal/value_type.h#L329-L352:

RBIMPL_ATTR_ARTIFICIAL()
static inline void
Check_Type(VALUE v, enum ruby_value_type t)
{
    if (RB_UNLIKELY(! RB_TYPE_P(v, t))) {
        goto slowpath;
    }
    else if (t != RUBY_T_DATA) {
        goto fastpath;
    }
    else if (rbimpl_rtypeddata_p(v)) {
        /* The intention itself is not necessarily clear to me, but at least it
         * is  intentional   to  rule   out  typed   data  here.    See  commit
         * a7c32bf81d3391cfb78cfda278f469717d0fb794. */
        goto slowpath;
    }
    else {
        goto fastpath;
    }

  fastpath:
    return;

  slowpath: /* <- :TODO: mark this label as cold. */
    rb_check_type(v, t);
}

Which in Ruby is trying to do some internal optimisations to make the resolution a bit faster, but then it might end up calling the rb_check_type() from error.c#L984-L997:

void
rb_check_type(VALUE x, int t)
{
    int xt;

    if (x == Qundef) {
    rb_bug(UNDEF_LEAKED);
    }

    xt = TYPE(x);
    if (xt != t || (xt == T_DATA && RTYPEDDATA_P(x))) {
    unexpected_type(x, xt, t);
    }
}

TruffleRuby also provides Check_Type(), albeit it's a macro (see: lib/cext/include/ruby/ruby.h#L664) that wraps rb_check_type(), as per:

#define Check_Type(v,t) rb_check_type((VALUE)(v),(t))

And the rb_check_type() is then defined (see: src/main/c/cext/object.c#L43-L45) as follows:

void rb_check_type(VALUE value, int type) {
  polyglot_invoke(RUBY_CEXT, "rb_check_type", rb_tr_unwrap(value), type);
}

I then found a bit of documentation on what might be happening internally:

cexts.md#c-runtime-implementation-details.

Should I go deeper into finding where things diverge, or would it be enough for you to decide whether this is something that you would consider something to potentially fix? Let me know. :)

Krzysztof

eregon commented 3 years ago

Could you copy-paste that in https://github.com/oracle/truffleruby/issues/new ? That's a great bug report :) We'll take it from there.

-pedantic

Right, that makes sense. -pedantic seems unnecessarily pedantic about warnings, it's probably better for everyone to not enable those warnings.

kwilczynski commented 3 years ago

Hi @eregon,

Could you copy-paste that in https://github.com/oracle/truffleruby/issues/new ?

Done! See: https://github.com/oracle/truffleruby/issues/2307

That's a great bug report :) We'll take it from there. [...]

Thank you! Meanwhile, I am going to look into adding some code to normalize this so that @gogainda could use this Ruby Gem without any issues on TruffleRuby.

Krzysztof

kwilczynski commented 3 years ago

Hi @gogainda, I believe we are ready to merge this.

With a number of recent changes we can successfully build and run against TruffleRuby, as per:

   kwilczynski@workstation  Ruby/ruby-magic   main ✔  $ ruby -v
truffleruby 21.0.0, like ruby 2.7.2, GraalVM CE Native [x86_64-linux]
   kwilczynski@workstation  Ruby/ruby-magic   main ✔  $ rake compile
Building ruby-magic using packaged libraries.
Static linking is disabled.
Cross build is disabled.
Using mini_portile version 2.5.0
Downloading file-5.39.tar.gz (100%)
Extracting file-5.39.tar.gz into tmp/x86_64-unknown-linux-gnu/ports/libmagic/5.39... OK
Running 'configure' for libmagic 5.39... OK
Running 'compile' for libmagic 5.39... OK
Running 'install' for libmagic 5.39... OK
Activating libmagic 5.39 (from /home/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/ports/x86_64-unknown-linux-gnu/libmagic/5.39)...
checking for ruby.h... yes
checking for rb_thread_call_without_gvl()... yes
checking for rb_thread_blocking_region()... no
checking for magic.h... yes
checking for -lmagic... yes
checking for magic_getpath()... yes
checking for magic_getflags()... yes
checking for utime.h... yes
checking for sys/types.h... yes
checking for sys/time.h... yes
checking for utime()... yes
checking for utimes()... yes
creating extconf.h
creating Makefile
compiling ../../../../ext/magic/functions.c
compiling ../../../../ext/magic/ruby-magic.c
linking shared-object magic/magic.so
   kwilczynski@workstation  Ruby/ruby-magic   main ✔  $ rake test
Loaded suite /home/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/.gems/gems/rake-13.0.3/lib/rake/rake_test_loader
Started
....................O
============================================================================================================================================================================================================================================================================================================================================================================
Omission: Platform does not support GC.compact [test_gc_compaction(MagicTest)]
/home/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/test/test_magic.rb:872:in `test_gc_compaction'
============================================================================================================================================================================================================================================================================================================================================================================
..............................................................................................................O
============================================================================================================================================================================================================================================================================================================================================================================
Omission: Platform does not support fork [test_magic_singleton_do_not_auto_load_global(MagicTest)]
/home/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/test/test_magic.rb:686:in `test_magic_singleton_do_not_auto_load_global'
============================================================================================================================================================================================================================================================================================================================================================================
............................
Finished in 1.033 seconds.
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
160 tests, 225 assertions, 0 failures, 0 errors, 0 pendings, 2 omissions, 0 notifications
100% passed
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
154.89 tests/s, 217.81 assertions/s
   kwilczynski@workstation  Ruby/ruby-magic   main ✔  $ rake compile -- --use-system-libraries
Building ruby-magic using system libraries.
checking for ruby.h... yes
checking for rb_thread_call_without_gvl()... yes
checking for rb_thread_blocking_region()... no
checking for magic.h... yes
checking for -lmagic... yes
checking for magic_getpath()... yes
checking for magic_getflags()... yes
checking for utime.h... yes
checking for sys/types.h... yes
checking for sys/time.h... yes
checking for utime()... yes
checking for utimes()... yes
creating extconf.h
creating Makefile
compiling ../../../../ext/magic/functions.c
compiling ../../../../ext/magic/ruby-magic.c
linking shared-object magic/magic.so
   kwilczynski@workstation  Ruby/ruby-magic   main ✔  $ rake test
Loaded suite /home/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/.gems/gems/rake-13.0.3/lib/rake/rake_test_loader
Started
....................O
============================================================================================================================================================================================================================================================================================================================================================================
Omission: Platform does not support GC.compact [test_gc_compaction(MagicTest)]
/home/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/test/test_magic.rb:872:in `test_gc_compaction'
============================================================================================================================================================================================================================================================================================================================================================================
..............................................................................................................O
============================================================================================================================================================================================================================================================================================================================================================================
Omission: Platform does not support fork [test_magic_singleton_do_not_auto_load_global(MagicTest)]
/home/kwilczynski/Development/Projects/Personal/Ruby/ruby-magic/test/test_magic.rb:686:in `test_magic_singleton_do_not_auto_load_global'
============================================================================================================================================================================================================================================================================================================================================================================
............................
Finished in 0.6579999999999999 seconds.
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
160 tests, 225 assertions, 0 failures, 0 errors, 0 pendings, 2 omissions, 0 notifications
100% passed
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
243.16 tests/s, 341.95 assertions/s

Things look good!

When you have a moment, if you could rebase against main and then update subject to read something along these lines (so that we know why the Travis CI configuration has been updated):

Update .travis.yml to allow builds on TruffleRuby

Thank you in advance!

Krzysztof

gogainda commented 3 years ago

@kwilczynski done

gogainda commented 3 years ago

Thank you for working on that:)

kwilczynski commented 3 years ago

Hi @gogainda,

Thank you for working on that:)

No problem!

Let me know if this works for you as intended.

Krzysztof