https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112020-04-01T18:54:37ZRuby Issue Tracking SystemRuby master - Misc #16750: Change typedef of VALUE for better type checkinghttps://bugs.ruby-lang.org/issues/16750?journal_id=848732020-04-01T18:54:37Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>Affecting 62,213 lines is huge. That's at least 62,213 places where an error could be introduced by using the incorrect union member or something similar.</p>
<p>If we were going to consider this, it would be helpful to have a list of previous commits that fixed bugs caused by the current <code>typedef</code> of <code>VALUE</code>? That would make it easier to assess the practical benefits of this proposal.</p>
<p>I think this change would require breaking compatibility with almost all existing C extensions, and I'm sure the benefits of this proposal do not exceed the costs of dropping C extension compatibility.</p> Ruby master - Misc #16750: Change typedef of VALUE for better type checkinghttps://bugs.ruby-lang.org/issues/16750?journal_id=848742020-04-01T22:54:42ZEregon (Benoit Daloze)
<ul></ul><p>I think the motivation to make C extensions safer is great here, but I see many problems with this approach.</p>
<p>TruffleRuby until recently typed <code>VALUE</code> (and <code>ID</code>) as <code>void*</code> (because TruffleRuby+Sulong store managed Java objects in there, and that used to require to be a pointer type, but no longer).<br>
This caused various incompatibilities, most notably <code>VALUE var; switch (var) { ...</code> failing to compile (the C compiler requires it to be an integer type to <code>switch</code> on it) and that's used in many gems.<br>
Bitwise operations on VALUE are also done in multiple gems to get a "hash code" of the VALUE (e.g., openssl, pg).<br>
So 2 weeks ago TruffleRuby changed to define VALUE as <code>unsigned long</code> just like MRI, which fixed all these incompatibilities:<br>
<a href="https://twitter.com/TruffleRuby/status/1240688301276684288" class="external">https://twitter.com/TruffleRuby/status/1240688301276684288</a></p>
<p>So I'm negative on changing that for compatibility, and I can say from experience it would break many gems.<br>
OTOH, I agree with your point that a better type such as <code>void*</code> or the <code>union</code> would find some incorrect usages.</p>
<p>There are actually multiple unexpected usages like that in MRI, including things like integer <a href="https://github.com/oracle/truffleruby/commit/0552ac72e3396492dc42ea53c1ac5ee2eb24a3ba#diff-ceeab34971467098f88189501912c870R1176" class="external">flags being typed as <code>VALUE</code></a>.<br>
I think those would be nice to fix regardless (but not very big issues either).</p>
<p>We found several bugs by having VALUE as <code>void*</code>, and tried to report them to the respective gems (VALUE used as an <code>int</code> variable, malloc(VALUE), arithmetic done on VALUE, etc).</p>
<p>I think <code>union</code> is a very dangerous construct (in general, it's very unsafe in C) and in this case it would be so easy to misuse (e.g., <code>value.as_basic->klass</code> on a Fixnum, OTOH <code>RBasic(value)</code> can check if meaningful).<br>
VALUE should remain an opaque pointer to let more freedom to the implementation.<br>
CRuby doesn't expose <code>struct</code> members in <code>include</code> in recent Ruby versions, and that's a good thing (e.g. never a good idea to dig in struct RString or RArray in a C extension).</p>
<p>I think something that could be interesting is having a preprocessor flag to type VALUE as <code>void*</code>.<br>
This might help C extension developers to find incorrect usages of <code>VALUE</code> (but at the cost of e.g. not allowing <code>switch (VALUE)</code>).<br>
Such an approach doesn't seem feasible for the <code>union</code>, because it would require to change lots of code, while swapping <code>unsigned long</code> and <code>void*</code> is not much diff at least in include files:<br>
<a href="https://github.com/oracle/truffleruby/commit/0552ac72e3396492dc42ea53c1ac5ee2eb24a3ba#diff-ceeab34971467098f88189501912c870" class="external">https://github.com/oracle/truffleruby/commit/0552ac72e3396492dc42ea53c1ac5ee2eb24a3ba#diff-ceeab34971467098f88189501912c870</a><br>
And also some changes in <a href="https://github.com/oracle/truffleruby/commit/4e127160f92fbccdcdd53981dc05d79800199b9d" class="external">https://github.com/oracle/truffleruby/commit/4e127160f92fbccdcdd53981dc05d79800199b9d</a></p> Ruby master - Misc #16750: Change typedef of VALUE for better type checkinghttps://bugs.ruby-lang.org/issues/16750?journal_id=848762020-04-02T00:35:43Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>FWIW ruby internally has <code>struct RVALUE</code> inside of gc.c, which is something similar to what is proposed in this ticket. It is at lest intentional to avoid the approach. The intention itself is not necessarily clear to me, though.</p> Ruby master - Misc #16750: Change typedef of VALUE for better type checkinghttps://bugs.ruby-lang.org/issues/16750?journal_id=848772020-04-02T04:01:51Zmatz (Yukihiro Matsumoto)matz@ruby.or.jp
<ul></ul><p>The current opaque approach has several reasons:</p>
<ul>
<li>
<p>portability<br>
The bit layout of bit fields in <code>struct</code> is somewhat implementation dependant, especially mixed with endianness. The <code>union</code> definition in OP may not work on big-endian CPUs.</p>
</li>
<li>
<p>performance<br>
The bit field operations may be far slower than simple bit operations on some compiler.</p>
</li>
</ul>
<p>I don't think it's worth it, especially for portability's sake.</p>
<p>Matz.</p> Ruby master - Misc #16750: Change typedef of VALUE for better type checkinghttps://bugs.ruby-lang.org/issues/16750?journal_id=848782020-04-02T12:03:33ZDan0042 (Daniel DeLorme)
<ul><li><strong>Tracker</strong> changed from <i>Feature</i> to <i>Misc</i></li></ul><p>Looks like I'm not able to add tags so... April fools :-)</p> Ruby master - Misc #16750: Change typedef of VALUE for better type checkinghttps://bugs.ruby-lang.org/issues/16750?journal_id=848792020-04-02T13:33:35Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul><li><strong>Assignee</strong> set to <i>10790</i></li></ul><p>No tags in Redmine now.</p> Ruby master - Misc #16750: Change typedef of VALUE for better type checkinghttps://bugs.ruby-lang.org/issues/16750?journal_id=848902020-04-03T02:34:17Zhsbt (Hiroshi SHIBATA)hsbt@ruby-lang.org
<ul><li><strong>Tags</strong> set to <i>joke</i></li></ul>