Handle NEWOBJ tracepoints settings fields#15320
Conversation
❌ 1/65368 Tests Failedtest/digest/test_digest.rb#test_race_mixed |
8500aad to
2e8f36c
Compare
| VALUE rb_wb_protected_newobj_of(struct rb_execution_context_struct *, VALUE, VALUE, uint32_t /* shape_id_t */, size_t); | ||
| VALUE rb_wb_unprotected_newobj_of(VALUE, VALUE, uint32_t /* shape_id_t */, size_t); |
There was a problem hiding this comment.
I'm not proud of this, but I don't think I can include shape.h here, or at least I don't know how to do it.
2e8f36c to
8420853
Compare
byroot
changed the title
|
The i686 failure looks real, but I have no environment to debug it :/ |
|
👋 @tenderlove & @jhawthorn would either of you have a bit of time to pair in the near future to figure out the i686 failure? (I still haven't acquired an x86 machine...) |
8420853 to
6336ea1
Compare
[Bug #21710] - struct.c: `struct_alloc` It is possible for a `NEWOBJ` tracepoint call back to write fields into a newly allocated object before `struct_alloc` had the time to set the `RSTRUCT_GEN_FIELDS` flags and such. Hence we can't blindly initialize the `fields_obj` reference to `0` we first need to check no fields were added yet. - object.c: `rb_class_allocate_instance` Similarly, if a `NEWOBJ` tracepoint tries to set fields on the object, the `shape_id` must already be set, as it's required on T_OBJECT to know where to write fields. `NEWOBJ_OF` had to be refactored to accept a `shape_id`.
6336ea1 to
ca4bc2b
Compare
| if (RB_TYPE_P(obj, T_STRING)) { | ||
| // Would fail !rb_obj_exivar_p(str) assertion in fstring_concurrent_set_create | ||
| return; | ||
| } |
There was a problem hiding this comment.
Hey, I'm curious about this one -- does this mean it's not OK to ask for rb_obj_id from strings? 👀
There was a problem hiding this comment.
Only in DEBUG mode, but to be fair the assertion in question should probably be updated.
Right now it checks !rb_obj_exivar_p which includes the object ID, but it should check that the object doesn't have ivars, object_id is a field, and ivars are fields, but not all fields are ivars.
The naming is a bit confusing too. I'll try to clean that up.
There was a problem hiding this comment.
Ah, perfect, thanks for clarifying! 🙏
|
@byroot This seems to have introduced a flaky crash in the parse.y CI on trunk. Could you look into this? https://github.com/ruby/ruby/actions/runs/19885611463/job/56992156948 History of the parse.y run: https://github.com/ruby/ruby/actions/workflows/parse_y.yml?query=branch%3Amaster+is%3Afailure |
|
Looking. |
3 participants
[Bug #21710]
struct_allocIt is possible for a
NEWOBJtracepoint call back to write fieldsinto a newly allocated object before
struct_allochad the timeto set the
RSTRUCT_GEN_FIELDSflags and such.Hence we can't blindly initialize the
fields_objreference to0we first need to check no fields were added yet.
rb_class_allocate_instanceSimilarly, if a
NEWOBJtracepoint tries to set fields on the object,the
shape_idmust already be set, as it's required on T_OBJECT toknow where to write fields.
NEWOBJ_OFhad to be refactored to accept ashape_id.