-
Notifications
You must be signed in to change notification settings - Fork 455
fix(gnovm): match Go's call-time dispatch for interface-bound method values #5737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ltzmaxwell
wants to merge
18
commits into
gnolang:master
Choose a base branch
from
ltzmaxwell:fix/defer12
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,151
−81
Open
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
73d25b5
test(gnovm): add failing filetest for value method bound to nil receiver
ltzmaxwell f041b0f
fix(gnovm): match Go's call-time dispatch for interface-bound method …
ltzmaxwell 0c130d4
chore(gnovm): keep .uverse alloc check minimal (nil-guard only); drop…
ltzmaxwell 6dc78e4
chore(gnovm): clarify recv's dual role in the deferred-call dispatch
ltzmaxwell f7a2ab5
test(gnovm): restore nil-receiver persist/reload panic test (per #573…
ltzmaxwell a2225a4
chore(gnovm): inline single-use valueMethodReceiver into the VPValMet…
ltzmaxwell 5d7c67c
fix(gnovm): detect cyclic embedded interface in lazy method-value dis…
ltzmaxwell ec58441
perf(gnovm): charge lazy-bound dispatch gas per hop, not per call
ltzmaxwell 431997f
docs(gnovm): update OpCPULazyBoundResolve comment for per-hop charge
ltzmaxwell ccb6c94
chore(gnovm): gofmt method_iface_defer.gno (align method decl braces)
ltzmaxwell 171a698
Merge branch 'master' into fix/defer12
ltzmaxwell aa5735d
Merge remote-tracking branch 'origin/master' into fix/defer12
ltzmaxwell 0019dc4
Merge remote-tracking branch 'mine/fix/defer12' into fix/defer12
ltzmaxwell dd77e41
fix(gnovm): detect struct-carried cycles in lazy method-value dispatch
ltzmaxwell 8d7e980
fix(gnovm): guard exportCopyValue against lazy bound-method nil Func,…
ltzmaxwell f5171d5
fix(gnovm): render lazy bound-method as selector, not <nil>, in bound…
ltzmaxwell 78853b3
docs(gnovm): note GetUnboundFunc returns nil for lazy interface binds
ltzmaxwell 9af6757
fix(gnovm): guard missing method in resolveLazyBound instead of gener…
ltzmaxwell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
43 changes: 43 additions & 0 deletions
43
gno.land/pkg/integration/testdata/method_iface_cyclic_persist.txtar
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # A cyclic embedded interface (`s.IG = s`) bound lazily through an interface | ||
| # (`G = o.Get`) and persisted must still raise the fatal cyclic-dispatch panic | ||
| # when reloaded and called in a later tx — i.e. cycle detection (keyed on the | ||
| # operand's reloaded identity) survives serialization and does NOT hang. The | ||
| # raw panic is recovered at the keeper boundary into a clean failed tx. | ||
|
|
||
| gnoland start | ||
|
|
||
| gnokey maketx addpkg -pkgdir $WORK/myrealm -pkgpath gno.land/r/test/myrealm -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stdout OK! | ||
|
|
||
| # bind G = o.Get over a cyclic s.IG = s and persist it; the bind itself is lazy | ||
| # (no resolution yet) so it must not panic here. | ||
| gnokey maketx call -pkgpath gno.land/r/test/myrealm -func Bind -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stdout OK! | ||
|
|
||
| # later tx: G is reloaded; calling it resolves the lazy bind, which loops over | ||
| # the reloaded cyclic operand and must detect the cycle (terminate, not hang). | ||
| ! gnokey maketx call -pkgpath gno.land/r/test/myrealm -func CallLater -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stderr 'cyclic embedded interface in method-value dispatch' | ||
|
|
||
| -- myrealm/gnomod.toml -- | ||
| module = "gno.land/r/test/myrealm" | ||
| gno = "0.9" | ||
|
|
||
| -- myrealm/myrealm.gno -- | ||
| package myrealm | ||
|
|
||
| type IG interface{ Get() int } | ||
|
|
||
| type Box struct{ IG } | ||
|
|
||
| var s *Box | ||
| var G func() int | ||
|
|
||
| func Bind(cur realm) { | ||
| s = &Box{} | ||
| s.IG = s // cyclic embedded interface | ||
| var o IG = s | ||
| G = o.Get // lazy bind, resolved at call time | ||
| } | ||
|
|
||
| func CallLater(cur realm) int { return G() } |
50 changes: 50 additions & 0 deletions
50
gno.land/pkg/integration/testdata/method_iface_lazy_persist.txtar
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| # A method value bound through an interface (G = o.Get) is lazy: it saves the | ||
| # operand and dynamically dispatches at call time. This must survive a | ||
| # persist/reload across txs — the stored bind re-reads the (mutated) embedded | ||
| # interface field at the later call and re-dispatches to the new concrete type. | ||
|
|
||
| gnoland start | ||
|
|
||
| gnokey maketx addpkg -pkgdir $WORK/lz -pkgpath gno.land/r/test/lz -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stdout OK! | ||
|
|
||
| # bind G = o.Get while s.Inner is Impl{1}; persist. | ||
| gnokey maketx call -pkgpath gno.land/r/test/lz -func Bind -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stdout OK! | ||
|
|
||
| # reassign s.Inner to Other{} (a different concrete type); persist. | ||
| gnokey maketx call -pkgpath gno.land/r/test/lz -func Mutate -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stdout OK! | ||
|
|
||
| # later tx: G is reloaded and called -> must re-dispatch to Other.Get -> 2. | ||
| gnokey maketx call -pkgpath gno.land/r/test/lz -func Call -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stdout '(2 int)' | ||
| stdout OK! | ||
|
|
||
| -- lz/gnomod.toml -- | ||
| module = "gno.land/r/test/lz" | ||
| gno = "0.9" | ||
|
|
||
| -- lz/lz.gno -- | ||
| package lz | ||
|
|
||
| type Impl struct{ x int } | ||
|
|
||
| func (i Impl) Get() int { return i.x } | ||
|
|
||
| type Other struct{} | ||
|
|
||
| func (Other) Get() int { return 2 } | ||
|
|
||
| type Inner interface{ Get() int } | ||
|
|
||
| type S struct{ Inner } | ||
|
|
||
| type Outer interface{ Get() int } | ||
|
|
||
| var s = &S{Inner: Impl{1}} | ||
| var G func() int | ||
|
|
||
| func Bind(cur realm) { var o Outer = s; G = o.Get } | ||
| func Mutate(cur realm) { s.Inner = Other{} } | ||
| func Call(cur realm) int { return G() } |
38 changes: 38 additions & 0 deletions
38
gno.land/pkg/integration/testdata/method_nil_value_persist.txtar
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # A value-receiver method bound to a nil pointer through an interface | ||
| # (`G = i.M`, i holding a typed-nil *T) must keep its call-time | ||
| # nil-pointer-deref panic across a persist/reload, matching Go. The | ||
| # receiver is stored as the nil *T itself (not a synthetic flag), so the | ||
| # timing survives serialization: G() reloaded in a later tx still panics. | ||
|
|
||
| gnoland start | ||
|
|
||
| gnokey maketx addpkg -pkgdir $WORK/myrealm -pkgpath gno.land/r/test/myrealm -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stdout OK! | ||
|
|
||
| # bind G = i.M (i holds a nil *T) and persist it; binding itself must not panic. | ||
| gnokey maketx call -pkgpath gno.land/r/test/myrealm -func Bind -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stdout OK! | ||
|
|
||
| # later tx: G is reloaded from store; calling it still derefs the nil *T | ||
| # and panics at call time (no "ZERO-RECEIVER", no OK!). | ||
| ! gnokey maketx call -pkgpath gno.land/r/test/myrealm -func CallLater -gas-fee 1000000ugnot -gas-wanted 8000000 -chainid=tendermint_test test1 | ||
| stderr 'nil pointer dereference' | ||
|
|
||
| -- myrealm/gnomod.toml -- | ||
| module = "gno.land/r/test/myrealm" | ||
| gno = "0.9" | ||
|
|
||
| -- myrealm/myrealm.gno -- | ||
| package myrealm | ||
|
|
||
| type I interface{ M() string } | ||
|
|
||
| type T struct{ x int } | ||
|
|
||
| func (T) M() string { return "ZERO-RECEIVER" } | ||
|
|
||
| var pt *T | ||
| var G func() string | ||
|
|
||
| func Bind(cur realm) { var i I = pt; G = i.M } | ||
| func CallLater(cur realm) string { return G() } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| # ADR-5737: Call-time dispatch for interface-bound method values | ||
|
|
||
| ## Status | ||
|
|
||
| Proposed. **Hard-fork-class** (changes a persisted value's wire format). | ||
|
|
||
| ## Context | ||
|
|
||
| In Go a method value formed through an interface (`g := i.M`, `defer i.M()`) | ||
| saves the operand at formation and materializes the concrete method + receiver | ||
| *inside the call*. GnoVM resolved it eagerly at bind, diverging from Go (vs | ||
| go1.25) on: nil-panic timing, value snapshot, embedded-method promotion, field | ||
| re-read through a boxed pointer, dynamic re-dispatch when an embedded interface | ||
| field is reassigned, and a VM crash for a nil embedded pointer-receiver (c5). | ||
| These last facets need the operand's *live* value at the call, so earlier narrow | ||
| fixes (a `nilReceiverPanic` flag, a `viaInterface` gate) couldn't reach them. | ||
|
|
||
| ## Decision | ||
|
|
||
| Bind interface method values lazily; resolve at the call. | ||
|
|
||
| - `BoundMethodValue` gains `Method Name`; `Func == nil` (operand in `Receiver`) | ||
| marks a lazy bind (`IsLazy()`). | ||
| - **Bind** (`VPInterface`): just save the operand — `&BoundMethodValue{Func: nil, | ||
| Receiver: *dtv, Method: name}`, no deref/dispatch. Concrete/pointer binds | ||
| (`pt.M`, `t.M`) are untouched. | ||
| - **Call** (`resolveLazyBound`, from `doOpPrecall` and the deferred call): walk | ||
| the operand's *current* value (`findEmbeddedFieldType` + `resolveInterfaceTrail`) | ||
| to the concrete method + receiver — the deref, embedded-field walk, copy, nil | ||
| panic, field re-read and re-dispatch all happen here. A nested embedded | ||
| interface yields another lazy bind; the loop unwraps it. | ||
| - **Defers** carry the callable on `Defer.Callable` (`*FuncValue` or | ||
| `*BoundMethodValue`), type-switched at the deferred call like `doOpPrecall`. | ||
| - **nil derefs** are raised by the walk (`GetPointerToFromTV`) via the `Run` | ||
| loop's panic path; a `VPSubrefField` nil guard makes c5 panic cleanly. | ||
| - **Reload**: `resolveLazyBound` fills the operand (`fillValueTV`) first, so a | ||
| persisted bind re-reads live state — what makes re-read / re-dispatch correct | ||
| across reload. | ||
| - `Func == nil` consumers audited (`IsCrossing`, `String`, GC, realm | ||
| persist/copy/fill, `GetShallowSize`). | ||
|
|
||
| ## Consequences | ||
|
|
||
| - **Hard fork**: `Method` is proto field 4 (`pb3_gen` regenerated); | ||
| `_allocBoundMethodValue` 200→216. Persisted bound methods change bytes → | ||
| different IAVL hashes / gas (`stdlib_restart_compare` pin moved). Ship only on | ||
| fresh genesis or a coordinated upgrade (like ADR-5544); old state still decodes | ||
| (new field defaults empty). | ||
| - **Behaviour**: interface method values match Go on every axis, across reload; | ||
| concrete/pointer binds unchanged. | ||
| - **Performance / gas.** Concrete path unchanged (`OpCPUPrecallBoundMethod` 199 | ||
| still valid). The dispatch *walk* moved from bind to call, so the two consts | ||
| were re-fit together (ratio-scaled; reference HW unavailable — see the | ||
| `TODO(calibration)` on each): | ||
| - bind `OpCPUSelectorInterface` 751 → **276** — the eager selector walked the | ||
| trail here (~751); the lazy bind only does the method lookup + lazy-bind | ||
| alloc (~140 ns ≈ 276), so leaving it at 751 would double-charge the walk. | ||
| - call `OpCPULazyBoundResolve` **529** (new) — the walk, now at call time. | ||
| Net ≈ +54 gas per interface method call (the genuine extra: a re-lookup + | ||
| throwaway bound-method alloc), not the ~480 an un-reduced base would | ||
| over-charge. `stdlib_restart_compare` pin → 2235788. A lean walk avoiding the | ||
| throwaway alloc was rejected — it would duplicate `GetPointerToFromTV`'s | ||
| dispatch/nil machinery on consensus code; the reuse form stays the single | ||
| source of truth. | ||
|
|
||
| ## Follow-ups | ||
|
|
||
| - **Calibration, before the fork ships** (both consts are ratio-scaled — the | ||
| reference HW was unavailable): re-measure `OpCPUSelectorInterface` (276) and | ||
| `OpCPULazyBoundResolve` (529) on the gas-table reference HW; and consider a | ||
| per-trail-step slope on the lazy resolve so deep/nested dispatch is metered | ||
| per hop, matching the eager path (currently flat, a bounded under-charge). | ||
| - Orthogonal, pre-existing (not caused or addressed here): interface method | ||
| *expressions* `I.M` rejected at preprocess (#5787); a method call on a *nil | ||
| interface* panics uncatchably (#5850). | ||
| - Cache a value-operand lazy bind's resolution (it is stable) to skip the | ||
| re-walk on repeated calls — **consensus-affecting, not just perf**: skipping | ||
| the walk drops its gas charge (`OpCPULazyBoundResolve` + alloc-gas), and | ||
| mutating a persisted bmv in place rewrites its bytes (merkle). So it is itself | ||
| a hard-fork change — fold into this fork's window or skip; it cannot ship as a | ||
| later rolling upgrade. Marginal benefit (only the call-a-stored-value-operand- | ||
| method-value-N-times pattern; pointer operands must never be cached, they | ||
| re-read live state), so deferred. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.