[ruby-core:124684] [Ruby Feature#21863] Add RBASIC_FLAGS() and RBASIC_SET_FLAGS()
Issue #21863 has been reported by Eregon (Benoit Daloze). ---------------------------------------- Feature #21863: Add RBASIC_FLAGS() and RBASIC_SET_FLAGS() https://bugs.ruby-lang.org/issues/21863 * Author: Eregon (Benoit Daloze) * Status: Open ---------------------------------------- Currently there is `RBASIC_CLASS()` which is the same as `RBASIC(obj)->klass` on CRuby but also checks it's called on a valid object: https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/i... I would like to add `RBASIC_FLAGS()` and `RBASIC_SET_FLAGS()`, which could be defined like this on CRuby: ```c #define RBASIC_FLAGS(obj) (RBASIC(obj)->flags) #define RBASIC_SET_FLAGS(obj, flags_to_set) (RBASIC(obj)->flags = flags_to_set) ``` This would let native extensions access those flags without relying on the specific fields, layout and existence of a `RBasic` `struct`. My main motivation here is that TruffleRuby, the alternative Ruby implementation with the most complete native extension support, cannot support having mutable fields in `RBasic`. The reason is because flags are mutable, it would be impossible to find out when the user writes to the flags if they write with just `RBASIC(obj)->flags = v`. And since the state related to flags is held internally on the Java object, writing to native memory would do nothing, which would be incompatible. With these macros we can know when the user writes to the flags and update the internal representation. In fact, TruffleRuby already implements these macros, since [this comment](https://github.com/truffleruby/truffleruby/issues/3118#issuecomment-18041941...). I would like to make it a standard C API to increase adoption and make it easier to discover. This would also be useful on CRuby to do additional checks when reading and writing flags: * We could add `RBIMPL_ASSERT_OR_ASSUME(!RB_SPECIAL_CONST_P(obj));` like `RBASIC_CLASS` does. * We could check that the frozen flag is not removed. * We could check that the frozen flag is consistent with the frozen flag on the object shape (if CRuby has a frozen flag in the shape). * We could check that the shape is not changed by native extensions this way (since the shape is stored in flags on 64-bit machines). * CRuby would be able to evolve the representation of RBasic more freely in the future. I'll note that there are already some macros/functions to manipulate flags such as `RB_FL_TEST()`, however they don't allow just reading or writing the flags: https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/i... However it seems clear that C extensions do read and write the RBasic->flags field currently and these `RB_FL_*()` macros/functions are not enough, so `RBASIC_FLAGS()` and `RBASIC_SET_FLAGS()` would be obvious replacements. Here is a gem-codesearch for `/RBASIC\(.+\)->flags/`: https://gist.github.com/eregon/5866bd86e626ae723b5b16d935af8f94 An alternative would be to deprecate the RBasic->flags field entirely and eventually hide it, and don't provide a replacement for direct reading/writing flags, but that would break a bunch of native extensions, so it does not seem actionable. With this proposal we won't break anything and we'll actually get finer control over accesses to flags if e.g. we want to prevent some invariant-breaking changes. -- https://bugs.ruby-lang.org/
Issue #21863 has been updated by Eregon (Benoit Daloze). Eregon (Benoit Daloze) wrote:
We could check that the frozen flag is consistent with the frozen flag on the object shape (if CRuby has a frozen flag in the shape).
Yes it does, so in CRuby the frozen flag is both stored in the shape and as a regular flag in `RBasic.flags`. So having RBASIC_CLASS() & RBASIC_SET_FLAGS() and also using them internally in CRuby would be a good way to check they stay consistent, which is notably important because the CRuby JITs rely on this (e.g. to avoid extra check when writing an `@ivar`, i.e. only check the shape and not the shape + flags, and probably the interpreter does the same if it inline caches the shape), and on the frozen flag not being cleared (ZJIT constant folds reads on frozen objects, cc @tekknolagi). ---------------------------------------- Feature #21863: Add RBASIC_FLAGS() and RBASIC_SET_FLAGS() https://bugs.ruby-lang.org/issues/21863#change-116299 * Author: Eregon (Benoit Daloze) * Status: Open ---------------------------------------- Currently there is `RBASIC_CLASS()` which is the same as `RBASIC(obj)->klass` on CRuby but also checks it's called on a valid object: https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/i... I would like to add `RBASIC_FLAGS()` and `RBASIC_SET_FLAGS()`, which could be defined like this on CRuby: ```c #define RBASIC_FLAGS(obj) (RBASIC(obj)->flags) #define RBASIC_SET_FLAGS(obj, flags_to_set) (RBASIC(obj)->flags = flags_to_set) ``` This would let native extensions access those flags without relying on the specific fields, layout and existence of a `RBasic` `struct`. My main motivation here is that TruffleRuby, the alternative Ruby implementation with the most complete native extension support, cannot support having mutable fields in `RBasic`. The reason is because flags are mutable, it would be impossible to find out when the user writes to the flags if they write with just `RBASIC(obj)->flags = v`. And since the state related to flags is held internally on the Java object, writing to native memory would do nothing, which would be incompatible. With these macros we can know when the user writes to the flags and update the internal representation. In fact, TruffleRuby already implements these macros, since [this comment](https://github.com/truffleruby/truffleruby/issues/3118#issuecomment-18041941...). I would like to make it a standard C API to increase adoption and make it easier to discover. This would also be useful on CRuby to do additional checks when reading and writing flags: * We could add `RBIMPL_ASSERT_OR_ASSUME(!RB_SPECIAL_CONST_P(obj));` like `RBASIC_CLASS` does. * We could check that the frozen flag is not removed. * We could check that the frozen flag is consistent with the frozen flag on the object shape (if CRuby has a frozen flag in the shape). * We could check that the shape is not changed by native extensions this way (since the shape is stored in flags on 64-bit machines). * CRuby would be able to evolve the representation of RBasic more freely in the future. I'll note that there are already some macros/functions to manipulate flags such as `RB_FL_TEST()`, however they don't allow just reading or writing the flags: https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/i... However it seems clear that C extensions do read and write the RBasic->flags field currently and these `RB_FL_*()` macros/functions are not enough, so `RBASIC_FLAGS()` and `RBASIC_SET_FLAGS()` would be obvious replacements. Here is a gem-codesearch for `/RBASIC\(.+\)->flags/`: https://gist.github.com/eregon/5866bd86e626ae723b5b16d935af8f94 An alternative would be to deprecate the RBasic->flags field entirely and eventually hide it, and don't provide a replacement for direct reading/writing flags, but that would break a bunch of native extensions, so it does not seem actionable. With this proposal we won't break anything and we'll actually get finer control over accesses to flags if e.g. we want to prevent some invariant-breaking changes. -- https://bugs.ruby-lang.org/
Issue #21863 has been updated by Eregon (Benoit Daloze). From https://ruby.esa.io/posts/144#[Feature%20#21863]%20Add%20RBASIC_FLAGS()%20and%20RBASIC_SET_FLAGS()%20(eregon)
@nobu: why are RB_FL_SET, RB_FL_TEST, RB_FL_UNSET not enough? They can be used for bit flags in flags field. Other data in flags (such as object type) must not be tweaked.
I posted a list of gems using it: https://gist.github.com/eregon/5866bd86e626ae723b5b16d935af8f94. It's not so many but I noticed the usage e.g. * in `numo-narray`, where RB_FL_* seems usable: https://github.com/ruby-numo/numo-narray/blob/95c05257349b954c027c2284834408... * in `allocation_tracer`, where I don't see an obvious replacement, cc @ko1: https://github.com/ko1/allocation_tracer/blob/f4ac1e8c15f827c293dc885cb0d52b... * in `heap_dump` * in `hpricot` How about deprecating the field then and indicating to use `RB_FL_*` instead? ---------------------------------------- Feature #21863: Add RBASIC_FLAGS() and RBASIC_SET_FLAGS() https://bugs.ruby-lang.org/issues/21863#change-116415 * Author: Eregon (Benoit Daloze) * Status: Open ---------------------------------------- Currently there is `RBASIC_CLASS()` which is the same as `RBASIC(obj)->klass` on CRuby but also checks it's called on a valid object: https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/i... I would like to add `RBASIC_FLAGS()` and `RBASIC_SET_FLAGS()`, which could be defined like this on CRuby: ```c #define RBASIC_FLAGS(obj) (RBASIC(obj)->flags) #define RBASIC_SET_FLAGS(obj, flags_to_set) (RBASIC(obj)->flags = flags_to_set) ``` This would let native extensions access those flags without relying on the specific fields, layout and existence of a `RBasic` `struct`. My main motivation here is that TruffleRuby, the alternative Ruby implementation with the most complete native extension support, cannot support having mutable fields in `RBasic`. The reason is because flags are mutable, it would be impossible to find out when the user writes to the flags if they write with just `RBASIC(obj)->flags = v`. And since the state related to flags is held internally on the Java object, writing to native memory would do nothing, which would be incompatible. With these macros we can know when the user writes to the flags and update the internal representation. In fact, TruffleRuby already implements these macros, since [this comment](https://github.com/truffleruby/truffleruby/issues/3118#issuecomment-18041941...). I would like to make it a standard C API to increase adoption and make it easier to discover. This would also be useful on CRuby to do additional checks when reading and writing flags: * We could add `RBIMPL_ASSERT_OR_ASSUME(!RB_SPECIAL_CONST_P(obj));` like `RBASIC_CLASS` does. * We could check that the frozen flag is not removed. * We could check that the frozen flag is consistent with the frozen flag on the object shape (if CRuby has a frozen flag in the shape). * We could check that the shape is not changed by native extensions this way (since the shape is stored in flags on 64-bit machines). * CRuby would be able to evolve the representation of RBasic more freely in the future. I'll note that there are already some macros/functions to manipulate flags such as `RB_FL_TEST()`, however they don't allow just reading or writing the flags: https://github.com/ruby/ruby/blob/86dba8cfaeabb3b86df921da24b3243b9ce4ab2a/i... However it seems clear that C extensions do read and write the RBasic->flags field currently and these `RB_FL_*()` macros/functions are not enough, so `RBASIC_FLAGS()` and `RBASIC_SET_FLAGS()` would be obvious replacements. Here is a gem-codesearch for `/RBASIC\(.+\)->flags/`: https://gist.github.com/eregon/5866bd86e626ae723b5b16d935af8f94 An alternative would be to deprecate the RBasic->flags field entirely and eventually hide it, and don't provide a replacement for direct reading/writing flags, but that would break a bunch of native extensions, so it does not seem actionable. With this proposal we won't break anything and we'll actually get finer control over accesses to flags if e.g. we want to prevent some invariant-breaking changes. -- https://bugs.ruby-lang.org/
participants (1)
-
Eregon (Benoit Daloze)