Truffleruby support

[deepj]

Hello @ohler55,
it would be great to support truffleruby by oj gem. I've reported one issue with truffleruby and oj, but it seems to be a bug in oj itself -> oracle/truffleruby#1437 (comment)

[eregon]

Specifically, the issue is that oj redefines rb_eEncodingError in some .c files, even if it is defined.
I think the extconf.rb should make a check specifically for rb_eEncodingError, and fallback to define it as rb_eException.

This fallback (if still needed for some old implementations) should be done in a header file like oj.h, and not C files to be consistent for the whole extension. For example, there is no fallback in custom.c.

Finally, branching on the Ruby implementation name and version as done in

oj/ext/oj/extconf.rb

Line 31 in a3cf482

'HAS_ENCODING_SUPPORT' => (('ruby' == type || 'rubinius' == type) &&

is very fragile. Instead, I believe feature detection should be used (or support for no longer supported Ruby versions removed).

[ohler55]

I'd be glad to help get Oj working on truffleruby. I'm curious about the compiler as it seems to identify errors that are not correct (dump.c:314). To "fix" those I need to be able to reproduce or have someone on your side run the compiles.

The encoding fallback can and should be moved, I agree and will make the change.

I haven't found any other way of dealing with some of the HAS or HAVE switches. You are correct that the extconf.rb needs some cleanup and there are some checks that exist in ruby/config.h. I'll make a pass at those as well.

[ohler55]

There is now a truffle branch. Please let me know what additional error you encounter.

[eregon]

@ohler55 That's great to hear, and the branch seems to simplify a lot of code :)

I'm curious about the compiler as it seems to identify errors that are not correct (dump.c:314).

It's clang version 4.0.1 (tags/RELEASE_401/final).
I think that warning at dump.c:314 is because the compiler doesn't know raise_invalid_unicode never returns. One way might be to annotate it as NORETURN(). Note dump_unicode() avoids the warning by doing cnt = 0; in the else branch which is another way.

I haven't found any other way of dealing with some of the HAS or HAVE switches.

Would have_func and similar from mkmf work for feature testing? I think that's the recommended way.

The changes for the switch don't seem to work for me, and would likely not work at runtime unfortunately.
I think handling switch on VALUE in general needs to be handled in TruffleRuby, as they are caused by VALUE being void* and not long:

rails.c:1022:10: error: expression is not an integer constant expression
    case (unsigned long)Qtrue:
         ^~~~~~~~~~~~~~~~~~~~
rails.c:1022:10: note: cast that performs the conversions of a reinterpret_cast is not allowed in a
      constant expression
rails.c:1023:10: error: expression is not an integer constant expression
    case (unsigned long)Qfalse:
         ^~~~~~~~~~~~~~~~~~~~~
rails.c:1023:10: note: cast that performs the conversions of a reinterpret_cast is not allowed in a
      constant expression
rails.c:1025:10: error: expression is not an integer constant expression
    case (unsigned long)Qnil:
         ^~~~~~~~~~~~~~~~~~~
rails.c:1025:10: note: cast that performs the conversions of a reinterpret_cast is not allowed in a
      constant expression
3 errors generated.

An alternative for this particular case is to avoid the switch, this works:

    state = RTEST(state) ? Qtrue : Qfalse;

and is also a bit simpler.
With that change on top of your branch, oj compiles on TruffleRuby without warnings :)

[ohler55]

I’ll take a look when I get connected. Traveling today.

…

On Nov 5, 2018, at 11:03, Benoit Daloze ***@***.***> wrote: @ohler55 That's great to hear, and the branch seems to simplify a lot of code :) I'm curious about the compiler as it seems to identify errors that are not correct (dump.c:314). It's clang version 4.0.1 (tags/RELEASE_401/final). I think that warning at dump.c:314 is because the compiler doesn't know raise_invalid_unicode never returns. One way might be to annotate it as NORETURN(). Note dump_unicode() avoids the warning by doing cnt = 0; in the else branch which is another way. I haven't found any other way of dealing with some of the HAS or HAVE switches. Would have_func and similar from mkmf work for feature testing? I think that's the recommended way. The changes for the switch don't seem to work for me, and would likely not work at runtime unfortunately. I think handling switch on VALUE in general needs to be handled in TruffleRuby, as they are caused by VALUE being void* and not long: rails.c:1022:10: error: expression is not an integer constant expression case (unsigned long)Qtrue: ^~~~~~~~~~~~~~~~~~~~ rails.c:1022:10: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression rails.c:1023:10: error: expression is not an integer constant expression case (unsigned long)Qfalse: ^~~~~~~~~~~~~~~~~~~~~ rails.c:1023:10: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression rails.c:1025:10: error: expression is not an integer constant expression case (unsigned long)Qnil: ^~~~~~~~~~~~~~~~~~~ rails.c:1025:10: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression 3 errors generated. An alternative for this particular case is to avoid the switch, this works: state = RTEST(state) ? Qtrue : Qfalse; and is also a bit simpler. With that change on top of your branch, oj compiles on TruffleRuby without warnings :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[ohler55]

If the latest changes work for you I'll plan on a release.

[eregon]

@ohler55 The extension compiles successfully with the latest oj, so it looks good to me, thank you for the fixes!

It doesn't run yet, but that's a different story and that should probably be fixed in TruffleRuby.
A release would be helpful in that it would allow at least bundle install to work if oj is in the Gemfile.

[ohler55]

Released

[eregon]

Thanks!