From 4d2a1c5b0b33ae881180fa7f706623d0dee1e7fc Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 27 Sep 2026 00:09:39 +0200 Subject: [PATCH 1/2] internal/vm: Marshal.dump carries the instance variables of String, Array, Hash and Regexp Marshal consulted only the encoding ivar of the kinds that have no ivar field of their own, so every variable Ruby code had set on a String, Array, Hash or Regexp was absent from the dump, and every such variable in a stream MRI wrote was discarded on load. Those kinds keep their variables in the VM-wide generic table (MRI's generic_iv_tbl_), which #674 added a few hours ago; before it existed a write to one of them was dropped in silence (#672), so Marshal had nothing to serialise and emitting nothing was indistinguishable from correct. It is not correct now: MRI's has_ivars reaches all of them through its `default: generic:` label and counts whatever rb_ivar_foreach reports. Dump side: writeIvarWrapped centralises MRI's w_object order -- the 'I' (TYPE_IVAR) byte, then w_uclass's 'e'/'C' prefixes, then the payload, then ONE ivar list in which the encoding ivar comes first (w_ivar subtracts what w_encoding wrote from the count it already emitted). TYPE_IVAR WRAPS the object; the wrapper takes no object-link id of its own, because w_remember runs before it on the dump side and r_object0 registers the wrapped object on the load side, so every back-reference is unaffected. A binary String or an Array with a variable but no encoding now takes the wrapper for the variable alone, which is what has_ivars does when encname is nil. The #_dump ('u') container gains the same treatment: its ivar set comes from the payload String alone, and the object is still remembered AFTER those variables, so it takes the higher link id MRI gives it. Load side: applyIvar's String and Regexp cases matched only :E and :encoding and had no default, so r_ivar's "everything sym2encidx does not recognise goes to rb_ivar_set" half was missing and every other variable fell through the switch. marshal.c read: w_object, w_objivar, w_ivar, w_ivar_each, w_encoding, encoding_name, has_ivars, obj_count_ivars, to_be_skipped_id, w_extended, w_class, w_uclass; r_object0/r_object_for, r_ivar, r_entry0, r_symlink, r_symreal. Co-Authored-By: Claude Opus 5 --- internal/vm/marshal_codec.go | 148 +++++++++++++++++++++++++++-------- 1 file changed, 115 insertions(+), 33 deletions(-) diff --git a/internal/vm/marshal_codec.go b/internal/vm/marshal_codec.go index 677eeed3..b1ccb0d4 100644 --- a/internal/vm/marshal_codec.go +++ b/internal/vm/marshal_codec.go @@ -134,12 +134,7 @@ func (d *mDumper) writeValue(v object.Value) { if d.link(x) { return } - d.writeExtended(x) - d.buf = append(d.buf, '[') - d.writeLong(len(x.Elems)) - for _, e := range x.Elems { - d.writeValue(e) - } + d.writeArray(x) case *object.Hash: if d.link(x) { return @@ -264,6 +259,64 @@ func (d *mDumper) writeEncodingIvar(e *marshalEncIvar) { d.writeValue(object.NewStringBytesEnc([]byte(e.name), "ASCII-8BIT")) } +// marshalIvars returns v's instance variables in first-assignment order and the +// table to read their values from, or nil when it has none to dump. +// +// It answers for the kinds with no ivar field of their own — String, Array, +// Hash, Regexp — whose variables live in the VM-wide generic table, MRI's +// generic_iv_tbl_. That table is what #674 added; before it, a write to one of +// those kinds was discarded in silence (#672), so Marshal had nothing to +// serialise and emitting nothing was indistinguishable from correct. It is +// correct no longer: MRI's has_ivars reaches every one of these kinds through +// its `default: generic:` label and counts what rb_ivar_foreach reports. +// +// The order pointer is non-nil for every kind routed here (genericIvars records +// the assignment order), so a nil one means there is nothing to report. +func marshalIvars(v object.Value) ([]string, map[string]object.Value) { + st := ivarStoreOf(v, false) + if len(st.tbl) == 0 || st.order == nil { + return nil, nil + } + names := make([]string, 0, len(*st.order)) + for _, n := range *st.order { + if _, live := st.tbl[n]; live { + names = append(names, n) + } + } + return names, st.tbl +} + +// writeIvarWrapped emits one value that may carry instance variables, an +// encoding, or both, in MRI's w_object order: the 'I' (TYPE_IVAR) byte, then +// w_uclass's 'e'/'C' prefixes, then the payload, then ONE ivar list covering +// both the encoding ivar and the object's own variables. +// +// TYPE_IVAR WRAPS the object; it is not a field of it. The count and the pairs +// come after the payload, and the encoding ivar comes first within them +// (w_ivar subtracts what w_encoding wrote from the count it already emitted). +// The 'I' byte takes no object-link id of its own: on the dump side w_remember +// runs before it, and on the load side r_object0 registers the wrapped object, +// so the wrapper is transparent to every back-reference. +func (d *mDumper) writeIvarWrapped(v object.Value, enc *marshalEncIvar, payload func()) { + names, tbl := marshalIvars(v) + n := len(names) + if enc != nil { + n++ + } + if n > 0 { + d.buf = append(d.buf, 'I') + } + d.writeExtended(v) + payload() + if n > 0 { + d.writeLong(n) + if enc != nil { + d.writeEncodingIvar(enc) + } + d.writeIvarPairs(names, tbl) + } +} + // writeExtended emits the 'e' (TYPE_EXTENDED) prefix naming each module // singleton-extended into v, mirroring MRI's w_extended (marshal.c) called with // check=TRUE: the singleton class is stepped over and every module between it @@ -296,18 +349,25 @@ func (d *mDumper) writeString(s *object.String) { if d.link(s) { return } - // MRI writes the 'I' ivar wrapper first, then w_uclass's 'e'/'C' prefixes, - // then the payload, then the ivar list (w_object in marshal.c). A binary - // string has no encoding ivar and so needs no wrapper. - enc := marshalEncodingIvar(s.EncName()) - if enc != nil { - d.buf = append(d.buf, 'I') - } - d.writeExtended(s) - d.writeStringPayload(s) - if enc != nil { - d.writeLong(1) - d.writeEncodingIvar(enc) + // A binary string with no instance variables has no encoding ivar either, so + // it needs no wrapper at all; one with either takes the 'I' container. + d.writeIvarWrapped(s, marshalEncodingIvar(s.EncName()), func() { d.writeStringPayload(s) }) +} + +// writeArray emits an Array: the 'I' wrapper when it carries instance +// variables (an Array is not encoding-capable, so it never has an encoding +// ivar), then the 'e' prefixes, then the elements, then the ivar list. +func (d *mDumper) writeArray(a *object.Array) { + d.writeIvarWrapped(a, nil, func() { d.writeArrayPayload(a) }) +} + +// writeArrayPayload emits just the TYPE_ARRAY tag and elements — the Array +// counterpart of writeStringPayload, so a 'C' container can own the wrapper. +func (d *mDumper) writeArrayPayload(a *object.Array) { + d.buf = append(d.buf, '[') + d.writeLong(len(a.Elems)) + for _, e := range a.Elems { + d.writeValue(e) } } @@ -323,7 +383,15 @@ func (d *mDumper) writeHash(h *object.Hash) { if !object.IsNil(h.DefaultProc) { raise("TypeError", "can't dump hash with default proc") } - d.writeExtended(h) + // A Hash is not encoding-capable, so the wrapper is the ivar list alone. It + // opens BEFORE the compare_by_identity 'C' container below, because MRI + // writes TYPE_IVAR before w_uclass and before that container. + d.writeIvarWrapped(h, nil, func() { d.writeHashPayload(h) }) +} + +// writeHashPayload emits the Hash body: the compare_by_identity 'C' container +// when it applies, then the entries and the default value. +func (d *mDumper) writeHashPayload(h *object.Hash) { // A Hash put into compare_by_identity mode carries no inline flag in the // stream, so MRI wraps it in a 'C' container naming the Hash class; loading // that container re-applies compare_by_identity. @@ -393,17 +461,10 @@ func (d *mDumper) writeRegexp(r *Regexp) { opt := regexpMarshalOpts(r) // As for a String: the 'I' wrapper, then w_uclass's 'e'/'C' prefixes, then the // payload, then the ivar list. A binary (ASCII-8BIT) Regexp carries no - // encoding ivar, so it is emitted bare. - enc := marshalEncodingIvar(regexpMarshalEnc(r)) - if enc != nil { - d.buf = append(d.buf, 'I') - } - d.writeExtended(r) - d.writeRegexpPayload(r, opt) - if enc != nil { - d.writeLong(1) - d.writeEncodingIvar(enc) - } + // encoding ivar, so one with no variables of its own is emitted bare. + d.writeIvarWrapped(r, marshalEncodingIvar(regexpMarshalEnc(r)), func() { + d.writeRegexpPayload(r, opt) + }) } // writeRegexpPayload emits just the TYPE_REGEXP tag, source and option byte — @@ -716,16 +777,29 @@ func (d *mDumper) writeUserDef(o *RObject) { // skips as "counted elsewhere" — and for a USERDEF container there is no // elsewhere, so the object's own variables are never written. Only the // String's contribute: its encoding, and any variable #_dump set on it. + // + // writeIvarWrapped is not used here: this container writes no 'e' prefix + // (MRI calls w_class with check=FALSE, so w_extended emits nothing), and the + // object must be remembered AFTER its variables, which is the reverse of + // every other container. enc := marshalEncodingIvar(str.EncName()) + names, tbl := marshalIvars(str) + n := len(names) if enc != nil { + n++ + } + if n > 0 { d.buf = append(d.buf, 'I') } d.buf = append(d.buf, 'u') d.writeSymbol(o.class.name) d.writeBytes(str.Str()) - if enc != nil { - d.writeLong(1) - d.writeEncodingIvar(enc) + if n > 0 { + d.writeLong(n) + if enc != nil { + d.writeEncodingIvar(enc) + } + d.writeIvarPairs(names, tbl) } d.remember(o) } @@ -1092,6 +1166,10 @@ func (r *mReader) readIvarWrapped() object.Value { func (r *mReader) applyIvar(base object.Value, name string, val object.Value) { switch b := base.(type) { case *object.String: + // Only :E and :encoding are the encoding; MRI's r_ivar sends everything + // sym2encidx does not recognise to rb_ivar_set. Without the default below + // each of those was dropped on the floor, so a String round-tripped + // through rbgo lost every variable MRI had written for it. switch name { case "E": if val.Truthy() { @@ -1103,6 +1181,8 @@ func (r *mReader) applyIvar(base object.Value, name string, val object.Value) { if s, ok := val.(*object.String); ok { b.Enc = s.Str() } + default: + setIvar(base, name, val) } case *Regexp: // The encoding ivar records the Regexp's source encoding, which #encoding @@ -1118,6 +1198,8 @@ func (r *mReader) applyIvar(base object.Value, name string, val object.Value) { if s, ok := val.(*object.String); ok { b.srcEnc = s.Str() } + default: + setIvar(base, name, val) } case *RObject: // A built-in subclass loaded from a 'C' container is ONE Ruby object: the From 8fd3d3e493c63677684a9b18f12890ce7ce5f624 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 27 Sep 2026 00:21:40 +0200 Subject: [PATCH 2/2] internal/vm: witnesses and guards for Marshal's instance-variable records 31 cases, every want value captured verbatim from the reference `ruby` (MRI 4.0.5) running the same source, so they witness MRI's bytes rather than transcribe rbgo's. 23 of them FAIL on 0450238 (verified by running this file unchanged in a clean origin/main worktree): String/Array/Hash/Regexp with one, several, nested, shared and removed variables; with an encoding and without; binary with a variable and no encoding (the wrapper for the variable alone); a compare_by_identity Hash; a module-extended receiver; the same variable-carrying object twice in one dump and one value referenced by two variables (the object-link table); the #_dump payload's variables and the link id they push the object to; and the load direction for each. 8 PASS before and after, and are guards rather than witnesses: a String with no variables, a binary String with none, an Array/Hash/Regexp with none, and the object's own variables NOT being written for a #_dump container. They exist because this fix could only break specs by emitting a record MRI does not expect. A mutation that moves the 'I' byte after writeExtended -- wrong TYPE_IVAR nesting, and it still compiles -- is caught by str_ext_and_ivar and ary_ext_ivar. Co-Authored-By: Claude Opus 5 --- internal/vm/marshal_wave46_test.go | 154 +++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 internal/vm/marshal_wave46_test.go diff --git a/internal/vm/marshal_wave46_test.go b/internal/vm/marshal_wave46_test.go new file mode 100644 index 00000000..86d04634 --- /dev/null +++ b/internal/vm/marshal_wave46_test.go @@ -0,0 +1,154 @@ +package vm_test + +import "testing" + +// TestMarshalWave46Ivars pins Marshal's handling of the instance variables of +// the kinds that have no ivar field of their own -- String, Array, Hash, Regexp +// -- and of the String a #_dump hook returns. +// +// Every want value was produced by running the SAME source under the reference +// `ruby` (MRI 4.0.5) and capturing its output verbatim, so these are witnesses +// to MRI's bytes rather than transcriptions of rbgo's. The generator refused any +// case MRI could not run. +// +// Cases marked "witness" FAIL on the code before this change: it consulted only +// the encoding ivar when dumping, and dropped every non-encoding ivar of a +// String or Regexp when loading. Cases marked "guard" PASS before and after -- +// they are here so that emitting a record MRI does not expect cannot pass +// unnoticed, which is the way a fix like this breaks specs that used to pass by +// omission. +func TestMarshalWave46Ivars(t *testing.T) { + cases := []struct{ name, src, want string }{ + // witness + {"str_one_ivar", + "s = \"ab\".dup; s.instance_variable_set(:@foo, \"bar\"); p Marshal.dump(s).bytes", + "[4, 8, 73, 34, 7, 97, 98, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 73, 34, 8, 98, 97, 114, 6, 59, 0, 84]\n"}, + // witness + {"str_three_ivars", + "s = \"ab\".dup; s.instance_variable_set(:@a, 1); s.instance_variable_set(:@b, :sym); s.instance_variable_set(:@c, [1]); p Marshal.dump(s).bytes", + "[4, 8, 73, 34, 7, 97, 98, 9, 58, 6, 69, 84, 58, 7, 64, 97, 105, 6, 58, 7, 64, 98, 58, 8, 115, 121, 109, 58, 7, 64, 99, 91, 6, 105, 6]\n"}, + // witness + {"str_usascii_ivar", + "s = \"ab\".dup.force_encoding(\"US-ASCII\"); s.instance_variable_set(:@foo, 1); p Marshal.dump(s).bytes", + "[4, 8, 73, 34, 7, 97, 98, 7, 58, 6, 69, 70, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // witness + {"str_latin1_ivar", + "s = \"ab\".dup.force_encoding(\"ISO-8859-1\"); s.instance_variable_set(:@foo, 1); p Marshal.dump(s).bytes", + "[4, 8, 73, 34, 7, 97, 98, 7, 58, 13, 101, 110, 99, 111, 100, 105, 110, 103, 34, 15, 73, 83, 79, 45, 56, 56, 53, 57, 45, 49, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // witness + {"str_binary_ivar_only", + "s = \"ab\".dup.force_encoding(\"binary\"); s.instance_variable_set(:@foo, 1); p Marshal.dump(s).bytes", + "[4, 8, 73, 34, 7, 97, 98, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // guard + {"str_binary_no_ivar", + "s = \"ab\".dup.force_encoding(\"binary\"); p Marshal.dump(s).bytes", + "[4, 8, 34, 7, 97, 98]\n"}, + // guard + {"str_no_ivar_utf8", + "p Marshal.dump(\"ab\".dup).bytes", + "[4, 8, 73, 34, 7, 97, 98, 6, 58, 6, 69, 84]\n"}, + // witness + {"str_ivar_nested_ivar", + "i = \"in\".dup; i.instance_variable_set(:@deep, 1); s = \"ou\".dup; s.instance_variable_set(:@inner, i); p Marshal.dump(s).bytes", + "[4, 8, 73, 34, 7, 111, 117, 7, 58, 6, 69, 84, 58, 11, 64, 105, 110, 110, 101, 114, 73, 34, 7, 105, 110, 7, 59, 0, 84, 58, 10, 64, 100, 101, 101, 112, 105, 6]\n"}, + // witness + {"str_removed_ivar", + "s = \"ab\".dup; s.instance_variable_set(:@a, 1); s.instance_variable_set(:@b, 2); s.send(:remove_instance_variable, :@a); p Marshal.dump(s).bytes", + "[4, 8, 73, 34, 7, 97, 98, 7, 58, 6, 69, 84, 58, 7, 64, 98, 105, 7]\n"}, + // witness + {"str_ext_and_ivar", + "module Mv; end; s = \"ab\".dup; s.instance_variable_set(:@foo, 1); s.extend(Mv); p Marshal.dump(s).bytes", + "[4, 8, 73, 101, 58, 7, 77, 118, 34, 7, 97, 98, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // witness + {"ary_ivar", + "a = [1]; a.instance_variable_set(:@foo, 1); p Marshal.dump(a).bytes", + "[4, 8, 73, 91, 6, 105, 6, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // guard + {"ary_no_ivar", + "p Marshal.dump([1]).bytes", + "[4, 8, 91, 6, 105, 6]\n"}, + // witness + {"ary_ext_ivar", + "module Mw; end; a = [1]; a.instance_variable_set(:@foo, 1); a.extend(Mw); p Marshal.dump(a).bytes", + "[4, 8, 73, 101, 58, 7, 77, 119, 91, 6, 105, 6, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // witness + {"hash_ivar", + "h = {1=>2}; h.instance_variable_set(:@foo, 1); p Marshal.dump(h).bytes", + "[4, 8, 73, 123, 6, 105, 6, 105, 7, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // witness + {"hash_default_ivar", + "h = Hash.new(9); h[1] = 2; h.instance_variable_set(:@foo, 1); p Marshal.dump(h).bytes", + "[4, 8, 73, 125, 6, 105, 6, 105, 7, 105, 14, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // witness + {"hash_identity_ivar", + "h = {}.compare_by_identity; h.instance_variable_set(:@foo, 1); p Marshal.dump(h).bytes", + "[4, 8, 73, 67, 58, 9, 72, 97, 115, 104, 123, 0, 6, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // guard + {"hash_no_ivar", + "p Marshal.dump({1=>2}).bytes", + "[4, 8, 123, 6, 105, 6, 105, 7]\n"}, + // witness + {"regexp_ivar", + "r = Regexp.new(\"ab\"); r.instance_variable_set(:@foo, 1); p Marshal.dump(r).bytes", + "[4, 8, 73, 47, 7, 97, 98, 0, 7, 58, 6, 69, 70, 58, 9, 64, 102, 111, 111, 105, 6]\n"}, + // guard + {"regexp_no_ivar", + "p Marshal.dump(Regexp.new(\"ab\")).bytes", + "[4, 8, 73, 47, 7, 97, 98, 0, 6, 58, 6, 69, 70]\n"}, + // witness + {"link_same_ivar_string_twice", + "s = \"ab\".dup; s.instance_variable_set(:@foo, \"bar\"); p Marshal.dump([s, s]).bytes", + "[4, 8, 91, 7, 73, 34, 7, 97, 98, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 73, 34, 8, 98, 97, 114, 6, 59, 0, 84, 64, 6]\n"}, + // witness + {"link_shared_ivar_value", + "v = \"sh\".dup; s = \"ab\".dup; s.instance_variable_set(:@a, v); s.instance_variable_set(:@b, v); p Marshal.dump(s).bytes", + "[4, 8, 73, 34, 7, 97, 98, 8, 58, 6, 69, 84, 58, 7, 64, 97, 73, 34, 7, 115, 104, 6, 59, 0, 84, 58, 7, 64, 98, 64, 6]\n"}, + // witness + {"udump_payload_ivar", + "class UDv; def _dump(d); s = \"pl\".dup; s.instance_variable_set(:@foo, \"bar\"); s; end; def self._load(x); new; end; end; p Marshal.dump(UDv.new).bytes", + "[4, 8, 73, 117, 58, 8, 85, 68, 118, 7, 112, 108, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 73, 34, 8, 98, 97, 114, 6, 59, 6, 84]\n"}, + // witness + {"udump_payload_ivar_link", + "class UDw; def _dump(d); s = \"pl\".dup; s.instance_variable_set(:@foo, \"bar\"); s; end; def self._load(x); new; end; end; o = UDw.new; p Marshal.dump([o, o]).bytes", + "[4, 8, 91, 7, 73, 117, 58, 8, 85, 68, 119, 7, 112, 108, 7, 58, 6, 69, 84, 58, 9, 64, 102, 111, 111, 73, 34, 8, 98, 97, 114, 6, 59, 6, 84, 64, 7]\n"}, + // guard + {"udump_object_ivar_not_dumped", + "class UDx; def initialize; @objiv = 1; end; def _dump(d); \"pl\".dup.force_encoding(\"binary\"); end; def self._load(x); new; end; end; p Marshal.dump(UDx.new).bytes", + "[4, 8, 117, 58, 8, 85, 68, 120, 7, 112, 108]\n"}, + // witness + {"load_str_ivar", + "s = Marshal.load(\"\\x04\\bI\\\"\\aab\\a:\\x06ET:\\t@fooi\\x06\"); p [s, s.encoding.to_s, s.instance_variable_get(:@foo)]", + "[\"ab\", \"UTF-8\", 1]\n"}, + // witness + {"load_str_ivar_no_enc", + "s = Marshal.load(\"\\x04\\bI\\\"\\aab\\x06:\\t@fooi\\x06\"); p [s, s.encoding.to_s, s.instance_variable_get(:@foo)]", + "[\"ab\", \"ASCII-8BIT\", 1]\n"}, + // witness + {"load_regexp_ivar", + "r = Marshal.load(\"\\x04\\bI/\\aab\\x00\\a:\\x06EF:\\t@fooi\\x06\"); p [r.source, r.encoding.to_s, r.instance_variable_get(:@foo)]", + "[\"ab\", \"US-ASCII\", 1]\n"}, + // guard + {"load_ary_ivar", + "a = Marshal.load(\"\\x04\\bI[\\x06i\\x06\\x06:\\t@fooi\\a\"); p [a, a.instance_variable_get(:@foo)]", + "[[1], 2]\n"}, + // guard + {"load_hash_ivar", + "h = Marshal.load(\"\\x04\\bI{\\x06i\\x06i\\a\\x06:\\t@fooi\\b\"); p [h, h.instance_variable_get(:@foo)]", + "[{1 => 2}, 3]\n"}, + // witness + {"load_link_ivar_string_twice", + "a = Marshal.load(\"\\x04\\b[\\aI\\\"\\aab\\a:\\x06ET:\\t@foo\\\"\\bbar@\\x06\"); p [a[0].equal?(a[1]), a[0].instance_variable_get(:@foo)]", + "[true, \"bar\"]\n"}, + // witness + {"roundtrip_str_ivar", + "s = \"ab\".dup; s.instance_variable_set(:@foo, \"bar\"); t = Marshal.load(Marshal.dump(s)); p [t, t.instance_variables, t.instance_variable_get(:@foo)]", + "[\"ab\", [:@foo], \"bar\"]\n"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := eval(t, c.src); got != c.want { + t.Errorf("src=%q\n got=%q\nwant=%q", c.src, got, c.want) + } + }) + } +}