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 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) + } + }) + } +}