fix reflection after DWARF symbol table changes in the rust compiler. - #719
Conversation
Since hubris switched to stable Rust 1.95.0, every command that
reads a MaybeUninit variable out of a target fails. `humility
sensors` falling back to its readmem backend reports "not an array:
Tuple(Tuple("MaybeDangling<[f32; 136]>", ...))" and prints nothing. The
release boundary are the hubris releases tagged all-sp-v1.0.72 and
all-sp-v1.73.0. The last good releases, relative to the humility DWARF
symbol reading bug fixed here, are all-sp-v1.0.71 and all-sp-v1.71.2.
Nothing in hubris changed. MaybeUninit<T> is a union whose value member
is ManuallyDrop<T>, and in rust 1.95.0 that type gained an inner
MaybeDangling<T>, so the reflected payload sits one level deeper than it
used to. load_union now peels the repr(transparent) newtypes the
compiler wraps the payload in, then presents the result the way older
toolchains produced on their own:
MaybeUninit<[u32; 154]> { value: [ ... ] }
Rebuilding that shape, rather than returning the bare payload, keeps the
type name we display and lets doppel's MaybeUninit keep verifying that
it was handed the type it expected. Output is unchanged on images from
either toolchain, and no longer leaks the compiler's internal type
names. Wrapper names are matched on the stem before `<`, so a hubris
type whose name merely starts with ManuallyDrop is not mistaken for the
compiler's.
Naming the struct as it is built leaves Value::set_name without callers,
so it goes: reassigning a name after the fact existed for this one case,
and it could never name the kinds that turn up here anyway, since arrays
and base types carry no name of their own.
Verified on against sidecar-sp across the release boundary and against a
live cosmo-a over the network, comparing released 0.13.2 with this build:
- all-sp-v1.71.2 works under both, so the fix stays compatible with
archives built by the old toolchain
- all-sp-v1.73.0 fails under 0.13.2 and works here
- the other twenty read-only commands in the sweep are unchanged
Fixes oxidecomputer/hubris#2615
No dump in the test corpus was built by a toolchain new enough to carry the MaybeUninit layout that hubris has produced since it moved to stable Rust 1.95.0. That is why CI could not have caught the decode break fixed in the previous commit; it took a report from the field. The dump added here comes from a sidecar-b-lab running all-sp-v1.76.0. The harness turns it into a case for each postmortem command, 29 in all. The one that matters is sensors-read: `sensors` against a dump uses the readmem backend, which is the path that failed, so that case fails on released humility and passes with the fix. Three cases are hand-edited to expect failure, because they want Gimlet state that a sidecar image does not have: - spd, which needs an SPD task - host last-panic, which needs host boot variables - a counters filter naming gimlet_seq Nothing in a core file records which board it came from or which toolchain built it, so the tests README grows a "Dump provenance" section for this one, and asks that later additions do the same. The README also states what this dump does not do. A frozen dump decodes the same way forever, so the corpus guards against humility regressions on the toolchains it already holds, but cannot catch the next change to how rustc represents a type; only a dump built by that toolchain can. Adding one when Hubris changes rust-toolchain.toml is the way to keep that window short.
Test record: hubris#2615Baseline throughout is released humility 0.13.2, compared against this branch. Release boundary
Corroborated without hardware: 1. Live reproduction, cosmo-a at 1.76.0, over the network0.13.2 fails with Plain 2. Command sweep, same target, 21 read-only commands, both binariesOnly 3. Release boundary on hardware, sidecar SP over SWDFlashed all-sp-v1.71.2, swept; flashed all-sp-v1.73.0, swept. 21 commands by 2
The other 20 commands exited identically across all four runs. v1.71.2 passing Output was compared, not only exit status. On v1.73.0 the released binary also 4. Final revision, sidecar SP over SWDThe committed revision rebuilds the canonical 5. Corpus coverageThe dump added in this PR was captured from the sidecar at 1.76.0. Released No pre-existing dump in the corpus shows this failure: every recorded Unrelated observation
Hardware stateThe sidecar SP was flashed v1.71.2, then v1.73.0, then restored to the 1.76.0 |
jamesmunns
left a comment
jamesmunns
left a comment
There was a problem hiding this comment.
This change looks reasonable to me, and I appreciate the on-hardware testing.
I did open a PR, #720, which I think might be a bit nicer than the current peel_transparent approach, as it avoids creating and then peeling the wrappers by just not making them in the first place. Either approach is likely fine though.
2 participants
Decode
MaybeUniniton hubris built with stable Rust.Fixes oxidecomputer/hubris#2615.
Hubris moved to stable Rust 1.95.0, which changed how
MaybeUninitislaid out in DWARF. humility decoded the old layout only. New format DWARF
entries failed to decode, including those read by
humility sensors. ThisPR changes the reflection code so that it accepts both layouts.
A dump built by the new toolchain is added to the suite to catch regressions.
Two commits:
The humility version is bumped to 0.13.5.
Testing
Beyond the checked-in tests, this was verified on lurch against real hardware:
reproduced on a live cosmo-a over the network, then confirmed across the
release boundary on sidecar-sp by flashing each release in turn and sweeping 21
read-only commands under both released 0.13.2 and this build. The last good
release passes under both, the first bad one fails only under 0.13.2, and no
other command changes.
Notes for review
readvaroutput is restored, not changed. 0.13.2 on new hubris leaks thecompiler's internal types into it; this branch prints what humility printed
before 1.95.0.
future changes to formats; a dump built by a future toolchain will be
required. The tests README now addresses that fact and asks for a dump
whenever hubris changes
rust-toolchain.toml.than naming its shape, which is what made this expensive to diagnose.
Summarizing it would help whatever drifts next.
it is wrong, so adding a dump from a new toolchain records whatever humility
does with it, failures included. Machine-readable output would let the suite
assert invariants instead of comparing snapshots, and detect the next drift
without depending on someone reading a diff carefully.