Skip to content

wave 29: Marshal's ivar and extend containers, Time's variables and zone round trip, String#unpack encodings (+27) - #648

Merged
tannevaled merged 4 commits into
mainfrom
wave29/marshal
Sep 25, 2026
Merged

tannevaled merged 4 commits into
mainfrom
wave29/marshal

Conversation

@tannevaled

Copy link
Copy Markdown
Contributor

Wave 29 — Marshal, Time and String#unpack

MRI 4.0.5 on arm64-darwin is the reference throughout. For Marshal every change
was checked against the bytes of the dump under both engines
(Marshal.dump(x).bytes), not against a successful round trip: a dump that
reloads in rbgo but not in MRI is a divergence a round-trip test cannot see.
That check is what caught two of the defects below, and rejected one change I
had already written.

Per file, before → after

file before after
core/marshal/dump_spec.rb 171 181
core/marshal/load_spec.rb 225 229
core/time/dup_spec.rb 5 6
core/time/new_spec.rb 113 115
core/string/unpack/a_spec.rb 31 33
core/string/unpack/b_spec.rb 30 32
core/string/unpack/h_spec.rb 30 32
core/string/unpack/u_spec.rb 25 27
core/string/unpack/x_spec.rb 21 22
core/string/unpack/z_spec.rb 14 15

Directory totals: core/marshal 417 → 431, core/time + core/symbol
778 → 781, core/numeric + core/string 3847 → 3857. +27 overall, no file
went down.
core/array/pack was measured too (1482, unchanged) because the
unpack work shares its directive table.

What was fixed, and the C source read for it

marshal.c — w_object, w_extended, w_class, w_uclass, w_ivar,
w_encoding, encoding_name, has_ivars, w_symbol

  • Extended modules on built-ins were not recorded. w_uclass calls
    w_extended, which writes an 'e' container per module. "cd".extend(Meths)
    dumped "\x04\bI\"\acd…" against MRI's "\x04\bIe:\nMeths\"\acd…".
  • …and were recorded where they must not be. w_class is called with
    check=FALSE for the marshal_dump ('U') and _dump ('u') containers,
    where w_extended emits nothing. rbgo emitted 'e' for those too. The two
    are opposite errors of one missing distinction.
  • A built-in subclass opened a nested 'I' wrapper instead of folding the
    payload's encoding into the container's single ivar list:
    "\x04\bC:\aUSI\"\aab…" against MRI's "\x04\bIC:\aUS\"\aab…". Per
    w_object the 'I' precedes the 'e'/'C' prefixes; per w_ivar the
    encoding ivar precedes the object's own.
  • A Struct's instance variables were dropped. has_ivars treats T_OBJECT
    as "counted elsewhere" but lets T_STRUCT fall through to the generic branch,
    so a Struct takes an outer 'I' wrapper rbgo never wrote.
  • SINGLETON_DUMP_UNABLE_P now reaches built-ins: s = "x"; def s.foo; end
    raises TypeError instead of silently dumping a bare String.
  • The 'u' container's ivar set comes from the payload String alone —
    has_ivars skips T_OBJECT, and for a USERDEF container there is no
    elsewhere. This was a regression from my own first commit, caught by measuring
    per file rather than in total.
  • w_remember position: the 'u' container registers its object only after
    its ivars, so those take the lower link ids.

time.c — time_mdump, time_mload; marshal.c — r_object0

  • A Time could not hold instance variables at all. instance_variable_set
    reported success and read back nil. Embedding methodValueState makes a
    Time a boxed value, the mechanism object_model.go already consults — so
    this needed no change outside my files. Time#frozen? starts working too,
    which is also what MRI does.
  • :zone and :offset: time_mdump writes the Time's own variables
    (rb_copy_generic_ivar), then :offset when not UTC, then :zone always.
    rbgo wrote :zone as nil for every non-UTC Time, losing the abbreviation MRI
    records. On load time_mload turns both into the Time's zone; rbgo dropped
    them, so Marshal.load(Marshal.dump(Time.new(2007,10,1,0,0,0,"-08:00"))) .utc_offset was 0 against MRI's -28800.
  • r_entry position on load: MRI passes an ivp flag into the
    TYPE_USERDEF branch so it reads its own variables and only then registers.
    rbgo registered first, so once the dump side started deferring, a repeated
    Time stopped linking — a real defect in the existing TestMarshal suite, not
    a spec-only one.
  • A Timezone object's #name is stored as-is, so its encoding is the user's.
    Re-tagging an ASCII-only name US-ASCII made a tzobj Time dump :E => false
    where MRI writes :E => true.

pack.c — pack_unpack_internal, utf8_to_uv

  • a/A/Z build their result with rb_str_new → ASCII-8BIT; b/B/
    h/H with rb_usascii_str_new → US-ASCII. All six were UTF-8.
  • Z* advances to just past the terminating NUL, not to the end of the
    data, so a repeated Z* walks successive strings.
    "a\0\0 b \0".unpack("Z*Z*Z*Z*") gave ["a","","",""] against MRI's
    ["a",""," b ",""].
  • U on a malformed sequence raises rather than yielding U+FFFD.
  • X* backs up by the bytes remaining, so it can run off the front:
    "abcd".unpack("CX*C") raises.

Also on load: the 'd' (TYPE_DATA) container, and an 'o' container now
refuses a class whose instances are not plain objects (a stream naming File
or Array), as obj_alloc_by_klass does.

A change the byte check rejected

I wrote a 'd' dump branch keyed on responding to #_dump_data. MRI
reaches TYPE_DATA from case T_DATA, so a pure-Ruby class defining
#_dump_data is still a T_OBJECT and dumps as 'o'. Marshal.dump(W.new)
had become "\x04\bd:\x06W…" against MRI's "\x04\bo:\x06W\x00". Reverted,
with the reason left at the site. The 'd' container is load-only.

The standing note on ivars on built-ins — MEASURED

It has been carried across several waves unchecked. Measured today, with both
engines on the same script:

receiver MRI rbgo (before) rbgo (after)
String 42, [:@probe] nil, [] nil, []
Array 42, [:@probe] nil, [] nil, []
Hash 42, [:@probe] nil, [] nil, []
Regexp 42, [:@probe] nil, [] nil, []
Time 42, [:@probe] nil, [] 42, [:@probe]
Object / Struct 42, [:@probe] same same

instance_variable_set returns the value and then the write is dropped:
ivarTable (object_model.go:2302) has no case for these receivers and returns
nil, so setIvar silently discards.

  • For String / Array / Hash / Regexp the note HOLDS and needs routing:
    object_model.go is not mine this wave. It blocks 13 further examples —
    8 in dump_spec (String/Array/Hash/Regexp "with instance variables" and the
    four "adds instance variables … into the objects table") and 5 in load_spec
    ("loads an array having ivar", "preserves hash ivars …", "loads a string with
    an ivar", and the two Regexp ivar examples).
  • For Time the note no longer holds — fixed here, and the briefs should stop
    repeating it. The fix needed no change outside time.go, because ivarTable
    already honours any value embedding methodValueState. The same one-line move
    is likely all String/Array/Hash/Regexp need, if their value types can carry it.

Other things found blocked in a sibling's file — for routing

  • dupValue has no *Time case (builtins.go), so Object#dup/#clone
    returned the receiver. Invisible until Time gained a frozen flag: with no flag
    to copy, "does not copy frozen status" passed for the wrong reason. Worked
    around by specialising only the copy on Time; the general fix is one case in
    dupValue.
  • Kernel#clone accepts a positional argument (builtins.go):
    Object.new.clone(1) returns a copy where MRI raises
    ArgumentError: wrong number of arguments (given 1, expected 0). Pre-existing
    and true for every receiver, so Time#clone matches the house behaviour here
    rather than being made uniquely strict.
  • String#force_encoding loses the subclass (encoding.go):
    US.new.force_encoding("binary").class is String, MRI says US. Blocks
    dump_spec's "dumps a String subclass" family (3).
  • Regexp cannot be subclassed — UR.new("ab").class is Regexp. Blocks
    4 dump_spec examples. (A Regexp subclass loaded from a 'C' container does
    work, and is tested.)
  • Symbol carries no encoding (internal/object): "→".b.to_sym.encoding
    is UTF-8, MRI says ASCII-8BIT. Blocks 2 dump + 2 load examples.
  • Time cannot be subclassed — blocks 2 dump_spec examples, including the
    anonymous-subclass TypeError.
  • Random has no marshal support (builtins.go) — 1 dump + 1 load.
  • String#crypt is entirely unimplemented — core/string/crypt_spec.rb is
    at pass=0 (6 examples). It is classic DES crypt(3); MRI on darwin matches
    the spec's vectors, so a pure-Go implementation would take all 6. Left for a
    later wave rather than started late in this one.

Not a defect: 3 dump_spec Exception examples are corpus drift

"dumps the message for the exception", "contains the filename in the backtrace"
and "dumps instance variables if they exist" expect :mesg"\bfoo with no
encoding ivar. MRI 4.0.5 itself fails that assertion — it produces
:mesgI"\bfoo\x06:\x06ET, which is exactly what rbgo produces. rbgo already
matches the reference; the vendored corpus is pinned to an older ruby/spec.
Changing rbgo to satisfy those three would make it diverge from MRI, so I left
them.

Gates

  • CGO_ENABLED=0 go build ./..., and GOOS=wasip1 GOARCH=wasm — clean.
  • CGO_ENABLED=0 GOWORK=off go test ./internal/vm/ — green. (The pre-existing
    failures of tests dialling 127.0.0.1:1 are unrelated.)
  • gofmt -l internal/vm/ — empty.
  • Coverage: the package reports 99.9%, unchanged from main, and the below-100
    list gains nothing — every function I added or changed in
    marshal_codec.go, time.go and pack.go is at 100%. Chasing the last
    branches is what found the 'C' container storing its encoding ivar as a
    literal variable named "E" (so a re-dump emitted the encoding twice) and the
    Timezone #name re-tagging — two real defects no spec in the corpus reaches,
    now pinned in TestMarshalWave29Coverage. One unreachable guard in remember
    was deleted rather than covered, and timeCloneFreezeArg's two "no keyword
    given" exits were merged into one rather than reached by a test asserting
    behaviour MRI does not have.
  • No timezone-dependent assertion was added. The one TZ-sensitive example
    (dump_spec "dumps the zone and the offset") stays failing: the vendored
    spec_helper.rb's with_timezone sets ENV['TZ'] to the bare name, so the
    offset the example wants is never established. I checked rbgo's TZ handling
    against MRI across all 15 forms the corpus uses and only the POSIX-offset form
    ("JST-9", "12") diverges — and the three specs using it already pass, so I
    did not implement it.

Tests

marshal_wave29_test.go and pack_wave29_test.go. Every expected value in the
table-driven cases was produced by running the same snippet under MRI 4.0.5 and
capturing its output verbatim — witnesses, not transcriptions. One test,
TestMarshalWave29DataLoad, pins a documented divergence rather than an MRI
witness, and says so at the site: rbgo has no T_DATA, so the 'd' branch is
chosen by the _dump_data/_load_data protocol instead of the allocated type.
The two agree on every stream MRI could have written and on both refusals the
suite checks.

🤖 Generated with Claude Code

tannevaled and others added 4 commits September 24, 2026 22:19
Read MRI marshal.c v3_4_0: w_object, w_extended, w_class, w_uclass,
w_ivar, w_encoding, encoding_name and has_ivars.

Four divergences, each with a byte witness against MRI 4.0.5:

- A module extended into a String/Array/Hash/Regexp/Range was not
  recorded at all. MRI's w_uclass calls w_extended, which emits an 'e'
  container per module. `"cd".extend(Meths)` dumped as "\x04\bI\"\acd..."
  where MRI writes "\x04\bIe:\nMeths\"\acd...".

- Conversely, w_class is called with check=FALSE for the #marshal_dump
  ('U') and #_dump ('u') containers, where w_extended emits NOTHING.
  rbgo emitted 'e' for those too, so an extended marshal_dump object
  dumped as "\x04\be:\nMethsU:\aUMi\x06" against MRI's "\x04\bU:\aUMi\x06".
  The two cases are opposite errors of one missing distinction, so both
  are fixed by moving writeExtended into the check=TRUE branches.

- A built-in subclass opened a nested 'I' wrapper around its payload
  instead of folding the payload's encoding into the container's single
  ivar list: "\x04\bC:\aUSI\"\aab\x06:\x06ET" where MRI writes
  "\x04\bIC:\aUS\"\aab\x06:\x06ET". Per w_object the 'I' precedes the
  'e'/'C' prefixes, and per w_ivar the encoding ivar precedes the
  object's own.

- A Struct's instance variables were dropped. has_ivars treats T_OBJECT
  as "counted elsewhere" but lets T_STRUCT fall through to the generic
  branch, so a Struct takes an outer 'I' wrapper that rbgo never wrote.

Also implements SINGLETON_DUMP_UNABLE_P for built-ins: a singleton with
its own methods or ivars is refused, so `s = "x"; def s.foo; end` now
raises TypeError instead of silently dumping a bare String.

The #_dump container additionally registers its object in the objects
table only AFTER its ivars (MRI's w_remember sits at the end of that
branch), so the ivars take the lower link ids, and takes the ivar set of
the String #_dump returned when that String has one — which is how a
_dump payload in a non-UTF-8 encoding carries its :encoding.

core/marshal/dump_spec.rb 171 -> 176 passing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read MRI time.c v3_4_0 (time_mdump, time_mload) and marshal.c (w_object's
TYPE_USERDEF branch).

A Time could not hold instance variables at all: `t.instance_variable_set`
reported success and read back nil, because a Time is a bare Go struct and
ivarTable has nothing to return for one. Embedding methodValueState makes
it a boxed value, which is the mechanism object_model.go already consults —
so this needed no change outside my files. Time#frozen? starts working as
a side effect, which is also what MRI does.

time_mdump copies the Time's own variables onto the payload String
(rb_copy_generic_ivar), then appends :offset — only when the Time is not
UTC — and :zone, which is written ALWAYS. rbgo wrote :zone as nil for every
non-UTC Time, losing the abbreviation MRI records ("AST", "CEST"), and
never wrote the Time's own variables. It now writes ivars, offset, zone in
time_mdump's order.

On load, time_mload deletes :offset and :zone from the payload
(rb_attr_delete) and turns them into the Time's zone, copying only what is
left back as real variables. rbgo dropped both, so a dumped Time came back
with utc_offset 0 whatever it was dumped with:

  Marshal.load(Marshal.dump(Time.new(2007,10,1,0,0,0,"-08:00"))).utc_offset
  MRI -28800, rbgo 0

The packed payload always holds the UTC wall clock (time_mdump calls
gmtimew), so re-displaying that instant in the recorded zone restores both
#utc_offset and #zone. :offset precedes :zone in the stream, which is why
the offset is applied first with no name — MRI's time_fixoff, after which a
Time on a bare numeric offset reports #zone as nil.

The Time container also registers itself in the objects table only after
its variables, like every other 'u' container, so
Marshal.dump([t, t, value]) now numbers the links as MRI does.

Time#dup and #clone: Object#dup and #clone are correct, but they build
their copy with dupValue, whose default branch returns the RECEIVER for a
Time — `t.dup.equal?(t)` was true. That was invisible until this commit,
because with no frozen flag to copy the "does not copy frozen status"
example passed for the wrong reason; making freeze work turned a latent
defect into a visible one. Specialising only the copy on Time keeps the
rest of the contract (initialize_dup / initialize_clone, the singleton
class, clone's freeze: keyword) where Object defines it. The general fix
is a *Time case in dupValue, which lives in builtins.go — a sibling's file
this wave.

core/marshal/dump_spec.rb 176 -> 178, load_spec.rb 225 -> 227,
core/time/dup_spec.rb 5 -> 6.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read MRI marshal.c v3_4_0: has_ivars, w_object's TYPE_USERDEF branch, and
r_object0's TYPE_USERDEF / TYPE_DATA / TYPE_OBJECT branches.

A REGRESSION FROM MY OWN FIRST COMMIT, caught by measuring per file rather
than in total. I had the 'u' container write the object's instance
variables. It must not: has_ivars skips T_OBJECT as "counted elsewhere",
and for a USERDEF container there IS no elsewhere, so an ordinary object's
variables are never written there. Only the payload String contributes —
its encoding, and anything #_dump set on it. That fix restores the three
examples the first commit broke and carries dump_spec past where it stood.

The 'u' container's ivars are also read back in the right place now. MRI
passes an `ivp` flag into the TYPE_USERDEF branch so it consumes its own
variables and only then calls r_entry, giving the object a HIGHER link id
than they have — the mirror of where w_remember sits. rbgo registered
first, so after the dump side started deferring, a repeated Time stopped
linking: `t = Time.at(5).utc; c = Marshal.load(Marshal.dump([t, t]));
c[0].equal?(c[1])` went false. The loader now defers to match. That was a
real defect in the existing TestMarshal suite, not a spec-only one.

Those variables also go where MRI puts them: on the payload String, before
_load is called with it, so :E / :encoding decide the encoding _load sees.
A _dump payload in windows-1251 now round-trips its encoding. Time is the
one exception, rebuilt directly rather than through Time._load, so its
variables are applied to the Time itself.

New: the 'd' (TYPE_DATA) container on load, for streams naming an object
that wraps a C pointer. MRI separates its two failures by the allocated
object's type; rbgo has no T_DATA, so it asks whether the class takes part
in the _dump_data / _load_data protocol. The limit of that approximation
is stated at TestMarshalWave29DataLoad rather than left implicit: the two
agree on every stream MRI could have written and on both refusals the
suite checks, and part only on a hand-built 'd' naming a pure-Ruby class.

There is deliberately NO 'd' dump branch. I wrote one keyed on responding
to #_dump_data and the byte witness rejected it: MRI reaches TYPE_DATA
from `case T_DATA`, so a pure-Ruby class defining #_dump_data is still a
T_OBJECT and dumps as 'o'. `Marshal.dump(W.new)` had become "\x04\bd:\x06W…"
against MRI's "\x04\bo:\x06W\x00". Comment left at the site.

An 'o' container may also only rebuild a plain object now: MRI allocates
and rejects a non-T_OBJECT, so a stream naming File or Array raises
ArgumentError instead of quietly building an instance of it.

Tests: marshal_wave29_test.go. Every expected value in TestMarshalWave29
was produced by running the same snippet under MRI 4.0.5 and capturing its
output verbatim, so they are witnesses rather than transcriptions.

core/marshal/dump_spec.rb 178 -> 181, load_spec.rb 227 -> 229.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… found

Read MRI pack.c v3_4_0: pack_unpack_internal's a/A/Z/b/B/h/H/U/X branches
and utf8_to_uv.

unpack tagged every string result UTF-8. MRI builds a/A/Z with rb_str_new,
which is ASCII-8BIT, and b/B/h/H with rb_usascii_str_new, which is
US-ASCII — a hex or bit string is all digits, a byte slice is not
characters at all. Six examples across three files turned on that alone.

'Z' with '*' advanced to the end of the data instead of to just past the
terminating NUL (`s = star ? t : s+len`), so a repeated Z* could not walk
successive strings: "a\0\0 b \0".unpack("Z*Z*Z*Z*") gave ["a", "", "", ""]
where MRI gives ["a", "", " b ", ""].

'U' yielded U+FFFD on a malformed sequence, because utf8.DecodeRune
substitutes where MRI's utf8_to_uv raises. "\xE3".unpack('U') returned
[65533]; it now raises ArgumentError, and the '*' form raises on the same
byte rather than walking past it.

'X' with '*' backed up a single byte. MRI sets len = send - s before the
bounds check, so it backs up by the bytes REMAINING and can run off the
front: "abcd".unpack("CX*C") raises.

core/string/unpack a 31->33, b 30->32, h 30->32, u 25->27, x 21->22,
z 14->15; six of the seven files now have no failures, and only the two
'p'/'P' pointer examples remain in the directory. core/array/pack was
measured alongside (1482, unchanged) since it shares the directive table.

Two further defects came out of closing the coverage gate, neither
reachable from any example in the corpus:

- A built-in subclass loaded from a 'C' container stored its encoding ivar
  as a REAL instance variable literally named "E", so re-dumping it emitted
  the encoding twice — "\x04\bIC:\bURx/\x00\x00\a:\x06EF;\x06F" against
  MRI's "…\x06:\x06EF". The container and its payload are one Ruby object,
  so :E / :encoding describe the payload.

- A Timezone object's #name was re-tagged US-ASCII. MRI stores the String
  that method returned as is, so an ASCII-only name from a UTF-8 literal
  dumps :E => true; rbgo wrote :E => false. Only a zone the Time carries as
  a location contributes the tz database's US-ASCII abbreviation.

Both are pinned in TestMarshalWave29Coverage. An unreachable guard in
remember was deleted rather than covered, and timeCloneFreezeArg's two
"no keyword given" exits were merged rather than reached by a test
asserting behaviour MRI does not have. Every function added or changed in
marshal_codec.go, time.go and pack.go is at 100%; the package total is
99.9%, unchanged from main.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tannevaled
tannevaled merged commit dda55ff into main Sep 25, 2026
7 checks passed
@tannevaled
tannevaled deleted the wave29/marshal branch September 25, 2026 07:04
tannevaled added a commit that referenced this pull request Sep 25, 2026
Three units, and this wave is mostly the line map paying out.

  #649  Harvesting what #645 made possible: owner-qualified backtrace labels
        (Object#foo where we printed foo), warn(uplevel:) prepending the
        caller's file and line — its scaffolding still carried the comment
        "rbgo has no caller line to prepend", untrue since yesterday — and
        Thread.each_caller_location.
  #650  Keywords decided at the CALL SITE, as MRI does: a trailing Hash is a
        positional argument unless the call site wrote it as keywords. The new
        flag is NEGATIVE on purpose — only sites the compiler can prove
        positional are flagged, so Object#send, Proc#call and define_method
        bodies keep the older behaviour rather than silently losing keywords.
        Also: post parameters bind BEFORE optionals (def q(a, b=9, *c, d) gave
        [1, nil, [], 2] where MRI gives [1, 9, [], 2]), a duplicated parameter
        name is a SyntaxError, and the implicit `it` block parameter.
  #648  Marshal's ivar and extend containers, Time's instance variables and
        the :zone/:offset round trip, String#unpack encodings         (+27)
  #647  dependency bump, green without intervention

Measured with scripts/conformance/rubyspec/run.sh against the pinned corpus at
87b1631, 16-way, TIMEOUT=25, three consecutive runs: 22227, 22226, 22226. The
floor is set eleven below the lowest — the margin rule the last four bumps
used.

One defect this wave exposed rather than introduced, now issue #651: three
parallel frame stacks are kept in step by hand, and FOUR sites truncate
frameNames alone. It went unnoticed while backtraces read that stack alone;
frameLabel now reads all three, so one leaked entry makes every later label
name a different frame. One site fixed in #649, three named and routed.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant