Feature #19057
openHide implementation of `rb_io_t`.
Description
In order to make improvements to the IO implementation like https://bugs.ruby-lang.org/issues/18455, we need to add new fields to struct rb_io_t
.
By the way, ending types in _t
is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop _t
where possible during this conversion.
Anyway, we should try to hide the implementation of struct rb_io
. Ideally, we don't expose any of it, but the problem is backwards compatibility.
So, in order to remain backwards compatibility, we should expose some fields of struct rb_io
, the most commonly used one is fd
and mode
, but several others are commonly used.
There are many fields which should not be exposed because they are implementation details.
Current proposal¶
The current proposed change https://github.com/ruby/ruby/pull/6511 creates two structs:
// include/ruby/io.h
#ifndef RB_IO_T
struct rb_io {
int fd;
// ... public fields ...
};
#else
struct rb_io;
#endif
// internal/io.h
#define RB_IO_T
struct rb_io {
int fd;
// ... public fields ...
// ... private fields ...
};
However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon (Benoit Daloze) + @tenderlovemaking (Aaron Patterson) have both given some kind of approval.
That being said, maybe it's not safe.
There are two alternatives:
Hide all details¶
We can make public struct rb_io
completely invisible.
// include/ruby/io.h
#define RB_IO_HIDDEN
struct rb_io;
int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state.
// internal/io.h
struct rb_io {
// ... all fields ...
};
This would only be forwards compatible, and code would need to feature detect like this:
#ifdef RB_IO_HIDDEN
#define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor
#else
#define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr)
#endif
Nested public interface¶
Alternatively, we can nest the public fields into the private struct:
// include/ruby/io.h
struct rb_io_public {
int fd;
// ... public fields ...
};
// internal/io.h
#define RB_IO_T
struct rb_io {
struct rb_io_public public;
// ... private fields ...
};
Considerations¶
I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us.
I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing.
Updated by ioquatix (Samuel Williams) about 1 year ago
Twitter discussion with @alanwu (Alan Wu): https://twitter.com/alanwusx/status/1581086330259668992
Updated by Eregon (Benoit Daloze) about 1 year ago
I support the Hide all details
approach.
With one tweak: not int rb_ioptr_descriptor(struct rb_io *ioptr);
but int rb_io_descriptor(VALUE io);
.
The goal is to not expose the struct in the C API, so let's not expose it (even if it becomes just an opaque pointer).
Not exposing structs in the C API makes it much easier to support the C API and evolve it.
For example it would make it easier to implement the C API with JNI or so in TruffleRuby, similar to HPy efforts in Python.
Having to emulate structs on GraalVM Sulong is additional complexity and makes C extensions slower to warm up (https://medium.com/graalvm/porting-matplotlib-from-c-api-to-hpy-aa32faa1f0b5).
If we go directly to the Hide all details
approach it might be more painful to transition though, i.e., more of a breaking change.
Maybe we can simply deprecate GetOpenFile
(which BTW does not have a proper RB_
prefix so more reason to deprecate), or deprecate rb_io_t
itself if that's possible?
And then of course we'd already add int rb_io_descriptor(VALUE io);
etc.
So for 1-2 releases we wouldn't break anything, just deprecation warnings and let more time for people to switch to int rb_io_descriptor(VALUE io);
etc.
Updated by ioquatix (Samuel Williams) about 1 year ago
I agree with Hide all details
but I'm more willing to compromise on the interface.
With one tweak: not int rb_ioptr_descriptor(struct rb_io *ioptr); but int rb_io_descriptor(VALUE io);.
Maybe it's CRuby problem, but there is a performance issue with such an implementation which has to extract and follow the pointer to the struct over and over again. If we can accept that, I'm okay with your proposal, but it's also a significant cost to compatibility. If we don't care about backwards compatibility and only forwards compatibility, I'm happy to support that.
Updated by jeremyevans0 (Jeremy Evans) about 1 year ago
- Tracker changed from Bug to Feature
- Backport deleted (
2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN)
Updated by kjtsanaktsidis (KJ Tsanaktsidis) 6 months ago
I did a bit of research on this topic this evening.
Firstly, some technical notes r.e. undefined behaviour.
My understanding is that "Current proposal" is really undefined behaviour. This is in the C standard, section 6.5.2.3, point 6 (http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf)
One special guarantee is made in order to simplify the use of unions: if a union contains several
structures that share a common initial sequence (see below), and if the union object currently contains
one of these structures, it is permitted to inspect the common initial part of any of them anywhere
that a declaration of the completed type of the union is visible. Two structures share a common initial
sequence if corresponding members have compatible types (and, for bit-fields, the same widths) for a
sequence of one or more initial members
It seems that this "common initial sequence" rule is really only for structures accessed through unions. Another fun gotcha is that, even if you do do all the accesses through a union, the compiler can assume that rb_io_public_t *
and rb_io_private_t *
should never alias each other (which is the exact opposite of what you want). E.g. - this program has a different result depending on whether optimizations are on or not - http://coliru.stacked-crooked.com/a/b57c8dd9e2ef3a02
I think to be technically correct, we would need to go for the "Nested public interface" approach - structs can be converted to a pointer to their first field according to the C standard (although I guess the reverse cast from rb_io_public_t *
back to rb_io_private_t *
would be UB?)
I don't think this matters though, because I agree that, looking at the contents of rb_io_t
, almost none of this should be public API, and we should strive to encapsulate it fully, like "Hide all details" (or like @Eregon (Benoit Daloze) suggested too).
I did a Github code search for usages of rb_io_t
, and all of the usages of it I could find basically fell into this pattern.
VALUE some_io = /* from somewhere */;
rb_io_t *fptr;
GetOpenFile(some_io, fptr)
// call an io.h method that takes rb_io_t *
rb_io_set_nonblock(fptr);
// or read the fd member and do something with that
do_something_external_with_fd(fptr->fd);
I think we could avoid 99% of the breakage, and still hide all the implementation details of rb_io_t, by basically redefining rb_io_t
to contain just the FD, and define a new internal, opaque type for internal IO use. We would do this like so:
Firstly, struct RFile
is currently in include/ruby/internal/core/rfile.h as:
struct rb_io_t;
struct RFile {
struct RBasic basic;
struct rb_io_t *fptr;
};
There are two spare words in there, and we would use one to add a pointer to a new structure. The definition of that structure would not be provided in public headers anyhere. So, we would change RFile in include/ruby/internal/core/rfile.h to be:
struct rb_io_t;
struct rb_io_impl;
struct RFile {
struct RBasic basic;
struct rb_io_t *fptr;
struct rb_io_impl *impl;
};
Then, we would change the definition of struct rb_io_t
in include/ruby/io.h to be:
typedef struct rb_io_t {
VALUE self;
int fd;
// _everything_ else is removed
} rb_io_t;
We would move all of its current contents to a new struct in internal/io.h:
struct rb_io_impl {
// All the juicy implementation details go here - _except_ no need to duplicate `self` and `fd`.
};
The current definition of GetOpenFile
essentially does RFILE(obj)->fptr
, so that would return the newly-slimmed-down public rb_io_t
. Attempts to read the file descriptor off that would continue to work.
Other methods in include/ruby/io.h which take a rb_io_t *
as an argument, like e.g. rb_io_set_nonblock
, would do RFILE(fptr->self)->impl
to get access to the implementation details struct. We could also define variants of these which took the VALUE
instead of the rb_io_t *
to avoid the pointer-chase of fptr->self
if desired.
The downside of this approach is that, as you identified, it costs an extra indirection in the implementation of most IO methods to obtain the impl from the VALUE. But, the benefits are that we get almost total encapsulation whilst maintaining backwards compatibility with almost all existing code. We can also shuffle fields between the private & public structs depending on observed compatibility issues found in the wild too.
Updated by Eregon (Benoit Daloze) 6 months ago
ioquatix (Samuel Williams) wrote in #note-3:
Maybe it's CRuby problem, but there is a performance issue with such an implementation which has to extract and follow the pointer to the struct over and over again.
It's pretty rare to access multiple fields of rb_io_t
in code I have seen, so then performance-wise it is the same if accessing a single field.
When accessing two fields it would be indeed an extra indirection, although I highly doubt this would be measurable overhead.
Coming from the other way, on all Ruby implementations besides CRuby, exposing struct rb_io *ioptr
would mean allocating a struct just to support those accesses, for every IO instance, possibly lazily. That's a much much bigger overhead than an extra pointer indirection.
As HPy shows, a fast and portable C API does not expose structs.
Updated by ioquatix (Samuel Williams) 6 months ago
Given the discussion here, I just want to aim for "Hide all the details".
Thanks everyone, the discussion has been most helpful.
I especially like the discussions around undefined behaviour and the example showing aliasing issues (did not know it could be possible haha).
Updated by ianks (Ian Ker-Seymer) 6 months ago
With one tweak: not int rb_ioptr_descriptor(struct rb_io *ioptr); but int rb_io_descriptor(VALUE io);.
The goal is to not expose the struct in the C API, so let's not expose it (even if it becomes just an opaque pointer).
I have mixed feelings about this. On one hand, it does successfully hide the implementation. On the other, casting pointers to unsigned ints has implications for pointer provenance models. Would be nice to get the opaqueness without the int-cast.
Updated by Eregon (Benoit Daloze) 6 months ago
ianks (Ian Ker-Seymer) wrote in #note-8:
With one tweak: not int rb_ioptr_descriptor(struct rb_io *ioptr); but int rb_io_descriptor(VALUE io);.
The goal is to not expose the struct in the C API, so let's not expose it (even if it becomes just an opaque pointer).On the other, casting pointers to unsigned ints has implications for pointer provenance models.
What do you mean? struct RBasic*
to VALUE
or vice-versa?
Almost every C API function does that, so that seems a separate issue.
Although maybe the above is not clear? int rb_io_descriptor(VALUE io);
would be called with the Ruby (IO) object (like almost every other C API function), not with a pointer (opaque or not) to the rb_io_t
struct.
Updated by ioquatix (Samuel Williams) 6 months ago
- Status changed from Open to Closed
https://github.com/ruby/ruby/pull/6511 was merged.
In addition, the following related PRs were merged:
- https://github.com/ruby/etc/pull/26
- https://github.com/ruby/io-wait/pull/25
- https://github.com/ruby/io-console/pull/43
A few notes:
- @Eregon (Benoit Daloze) was mostly correct in that most code was "Get the file descriptor and do something". Very few code paths were getting
fptr
and doing multiple things. - This does break some backwards compatibility but this is considered to be acceptable as we essentially want to hide the implementation and clean up legacy code paths.
- This allows us to restructure some of the internals of the
IO
implementation for improved semantics and performance. - We will probably want to continue the effort to clean up some of the legacy APIs, e.g.
GetOpenFile
andMakeOpenFile
etc. - We probably want to continue to deprecate and remove
fptr
style functions.
Updated by hsbt (Hiroshi SHIBATA) 6 months ago
- Related to Bug #19704: Unable to install readline-ext since 18e55fc1e1ec20e8f3166e3059e76c885fc9f8f2 added
Updated by ioquatix (Samuel Williams) 6 months ago
Unfortunately this was reverted due to some extensions no longer compiling.
I think it's expected that some dependencies using internal details of rb_io_t
should be fixed.
Updated by ioquatix (Samuel Williams) 6 months ago
- Status changed from Closed to Assigned
Updated by Eregon (Benoit Daloze) 6 months ago
I am not sure there is an easier path here, IMO it's OK for some extensions to break while they test against ruby-head, and to adapt to it (and we are not close to December).
It would be nicer to deprecate first, but I am not sure how feasible is that.
It seems maybe GCC & Clang might support deprecating struct members: https://stackoverflow.com/questions/22823477/c-portable-way-to-deprecate-struct-members
Then we could deprecate every member of rb_io_t
.
That seems useful, even if the warning only works on those compilers, most people would see it.
That said, even with deprecations for one release I'm pretty sure not all extensions using rb_io_t members will have migrated, so there will be some breakage anyway, just like for every deprecated thing.
But if extensions didn't migrate for more than a year, then it's really on them if they did not address it in time.
Another way could be to deprecate GetOpenFile
(or the whole struct type if that's possible).
But that seems difficult because many rb_io_*
functions take a rb_io_t*, e.g., FILE *rb_io_stdio_file(rb_io_t *fptr);
, so we would then need to add a variant taking VALUE io
for all of these (with another name).
There is less value in that, because if rb_io_t
is opaque these functions are not really problematic, e.g. TruffleRuby could just return and cast the io
for GetOpenFile(io)
, and it would require larger efforts to migrate.
The last paragraph of https://bugs.ruby-lang.org/issues/19057#note-2 was mentioning similar ideas about deprecation.
Updated by ioquatix (Samuel Williams) 6 months ago
I'm in favour of eventually deprecating rb_io_t
and I think that means anything related to it.
I think deprecations can make things a little easier but I'm not against breaking things that are out of our control.
I'm happy to fix things as they come up. Today we fixed nio4r, and polyphony, along with readline-ext
and the other io-*
gems.
I agree, once we stop exposing rb_io_t
, even CRuby can take the same approach (just return the IO
value). Whether we want to or not is another question, but I imagine only a few VALUE io
functions are missing now.
Updated by ioquatix (Samuel Williams) 6 months ago
Okay, here is a PR to introduce deprecations: https://github.com/ruby/ruby/pull/7916
Updated by byroot (Jean Boussier) 6 months ago
@ioquatix (Samuel Williams) I'm not sure which change exactly is the cause, but it appears that the recent rb_io_t
changes broke raindrops
current directory: /usr/local/lib/ruby/gems/3.3.0+0/gems/raindrops-0.20.1/ext/raindrops
make DESTDIR\= sitearchdir\=./.gem.20230609-25-giurc0 sitelibdir\=./.gem.20230609-25-giurc0 clean
current directory: /usr/local/lib/ruby/gems/3.3.0+0/gems/raindrops-0.20.1/ext/raindrops
make DESTDIR\= sitearchdir\=./.gem.20230609-25-giurc0 sitelibdir\=./.gem.20230609-25-giurc0
compiling linux_inet_diag.c
In file included from /usr/include/x86_64-linux-gnu/bits/libc-header-start.h:33,
from /usr/include/stdio.h:27,
from /usr/local/include/ruby-3.3.0+0/ruby/defines.h:16,
from /usr/local/include/ruby-3.3.0+0/ruby/ruby.h:25,
from /usr/local/include/ruby-3.3.0+0/ruby.h:38,
from linux_inet_diag.c:1:
/usr/include/features.h:187:3: warning: #warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE" [-Wcpp]
187 | # warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE"
| ^~~~~~~
In file included from linux_inet_diag.c:4:
my_fileno.h:9:7: warning: "HAVE_RB_IO_T" is not defined, evaluates to 0 [-Wundef]
9 | #if ! HAVE_RB_IO_T
| ^~~~~~~~~~~~
my_fileno.h:16:8: warning: "HAVE_RB_IO_T" is not defined, evaluates to 0 [-Wundef]
16 | # if !HAVE_RB_IO_T || (RUBY_VERSION_MAJOR == 1 && RUBY_VERSION_MINOR == 8)
| ^~~~~~~~~~~~
my_fileno.h: In function ‘my_fileno’:
my_fileno.h:10:19: error: unknown type name ‘OpenFile’; did you mean ‘GetOpenFile’?
10 | # define rb_io_t OpenFile
| ^~~~~~~~
my_fileno.h:25:2: note: in expansion of macro ‘rb_io_t’
25 | rb_io_t *fptr;
| ^~~~~~~
In file included from my_fileno.h:3,
from linux_inet_diag.c:4:
/usr/local/include/ruby-3.3.0+0/ruby/io.h:395:55: warning: assignment to ‘int *’ from incompatible pointer type ‘struct rb_io *’ [-Wincompatible-pointer-types]
395 | #define RB_IO_POINTER(obj,fp) rb_io_check_closed((fp) = RFILE(rb_io_taint_check(obj))->fptr)
| ^
/usr/local/include/ruby-3.3.0+0/ruby/io.h:401:21: note: in expansion of macro ‘RB_IO_POINTER’
401 | #define GetOpenFile RB_IO_POINTER
| ^~~~~~~~~~~~~
my_fileno.h:30:2: note: in expansion of macro ‘GetOpenFile’
30 | GetOpenFile(io, fptr);
| ^~~~~~~~~~~
/usr/local/include/ruby-3.3.0+0/ruby/io.h:395:55: warning: passing argument 1 of ‘rb_io_check_closed’ from incompatible pointer type [-Wincompatible-pointer-types]
395 | #define RB_IO_POINTER(obj,fp) rb_io_check_closed((fp) = RFILE(rb_io_taint_check(obj))->fptr)
| ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| int *
/usr/local/include/ruby-3.3.0+0/ruby/io.h:401:21: note: in expansion of macro ‘RB_IO_POINTER’
401 | #define GetOpenFile RB_IO_POINTER
| ^~~~~~~~~~~~~
my_fileno.h:30:2: note: in expansion of macro ‘GetOpenFile’
30 | GetOpenFile(io, fptr);
| ^~~~~~~~~~~
/usr/local/include/ruby-3.3.0+0/ruby/io.h:638:34: note: expected ‘rb_io_t *’ {aka ‘struct rb_io *’} but argument is of type ‘int *’
638 | void rb_io_check_closed(rb_io_t *fptr);
| ~~~~~~~~~^~~~
In file included from linux_inet_diag.c:4:
my_fileno.h:17:41: error: request for member ‘f’ in something not a structure or union
17 | # define FPTR_TO_FD(fptr) fileno(fptr->f)
| ^~
my_fileno.h:31:7: note: in expansion of macro ‘FPTR_TO_FD’
31 | fd = FPTR_TO_FD(fptr);
| ^~~~~~~~~~
linux_inet_diag.c: At top level:
cc1: warning: unrecognized command line option ‘-Wno-self-assign’
cc1: warning: unrecognized command line option ‘-Wno-parentheses-equality’
cc1: warning: unrecognized command line option ‘-Wno-constant-logical-operand’
make: *** [Makefile:248: linux_inet_diag.o] Error 1
Aside from the warnings (which do seem bad) the hard error seem to come from:
#include <ruby.h>
#ifdef HAVE_RUBY_IO_H
# include <ruby/io.h>
#else
# include <stdio.h>
# include <rubyio.h>
#endif
#if ! HAVE_RB_IO_T
# define rb_io_t OpenFile
#endif
Updated by ioquatix (Samuel Williams) 6 months ago
Thanks for the report, fixed in https://github.com/ioquatix/raindrops/commit/94dbdd94977d895f98c084d0ca31c2b9cf0d25d3
Updated by kamil-gwozdz (Kamil Gwóźdź) 5 months ago
byroot (Jean Boussier) wrote in #note-17:
@ioquatix (Samuel Williams) I'm not sure which change exactly is the cause, but it appears that the recent
rb_io_t
changes brokeraindrops
I've noticed the same thing while trying to install kgio
with the latest ruby HEAD.
Updated by k0kubun (Takashi Kokubun) 5 months ago
I've noticed the same thing while trying to install kgio with the latest ruby HEAD.
It looks like @byroot (Jean Boussier) sent a patch for this at https://yhbt.net/kgio-public/B5843088-6EC1-4D1B-A45A-2699CA75DD7F@gmail.com/T/#u.
Both patches are still not released. Unless @normalperson (Eric Wong) cuts a timely release for kgio and raindrops, we will not be able to use Unicorn on Ruby 3.3 because of this Feature.
Updated by nobu (Nobuyoshi Nakada) 5 months ago
k0kubun (Takashi Kokubun) wrote in #note-20:
It looks like @byroot (Jean Boussier) sent a patch for this at https://yhbt.net/kgio-public/B5843088-6EC1-4D1B-A45A-2699CA75DD7F@gmail.com/T/#u.
rb_convert_type(io, T_FILE, "IO", "to_io")
can be rb_io_get_io(io)
.
Updated by k0kubun (Takashi Kokubun) 4 months ago
cool.io
is another gem broken by this feature. FYI, I filed a pull request here https://github.com/tarcieri/cool.io/pull/79.
Updated by ioquatix (Samuel Williams) 3 months ago
Status of several projects that are affected by this change:
-
cool.io
has since been fixed and released. -
unicorn
has merged a fix but it is not released: https://yhbt.net/unicorn.git/63c85c4282d15e22bd32a905883d2d0e149619d1/s/ -
kgio
has a patch submitted but it is not merged yet: https://yhbt.net/kgio-public/B5843088-6EC1-4D1B-A45A-2699CA75DD7F@gmail.com/T/#u - @normalperson (Eric Wong) has said they will merge it: https://yhbt.net/kgio-public/20230611213930.M342144@dcvr/T/#u -
raindrops
has merged a fix but it is not released: https://yhbt.net/raindrops-public/20230611213328.379546-1-bofh@yhbt.net/T/#t
Updated by naruse (Yui NARUSE) 3 months ago
I'll release Ruby 3.3.0 preview 2 soon.
I'm concerning that those three projects don't support the preview yet.
A preview release is to allow people to test their applications, but without support of those foundational projects it's hard to achieve the goal.
If those projects are not fixed before the preview release, I'll revert changes related to Feature #19057 for preview 2.
Updated by ioquatix (Samuel Williams) 3 months ago
@naruse (Yui NARUSE) Here is the compatibility fix that will allow kgio
, unicorn
and so on to compile with no changes: https://github.com/ruby/ruby/pull/8286
Please feel free to merge that before doing the preview 2 release, if @normalperson (Eric Wong) has not released updated gems.
However, bear in mind this delays the inevitable - I'd still like to propose we remove this completely in 3.4 - 3.3 has deprecations, and 3.4 is removed. Does that sound okay to you?
Updated by byroot (Jean Boussier) 3 months ago
Eric just said he'll cut new unicorn
and kgio
releases: https://yhbt.net/kgio.git/dbf5290cf9f89174f6b35a597af9a4226633d79b/s/
Will push out a release once docs are updated to more strongly
discourage the use of kgio and unicorn.