From e34a322859250dc4d440a0fa19a88d448112ab2f Mon Sep 17 00:00:00 2001 From: deelawn Date: Wed, 27 Mar 2024 16:16:23 -0700 Subject: [PATCH 01/14] don't allow max cycle configuration --- gno.land/cmd/gnoland/start.go | 10 +--------- gno.land/pkg/gnoland/app.go | 5 ++--- gno.land/pkg/gnoland/node_inmemory.go | 9 +++------ gno.land/pkg/sdk/vm/common_test.go | 2 +- gno.land/pkg/sdk/vm/keeper.go | 2 +- 5 files changed, 8 insertions(+), 20 deletions(-) diff --git a/gno.land/cmd/gnoland/start.go b/gno.land/cmd/gnoland/start.go index 2b1757706f8..7fa7de32e5c 100644 --- a/gno.land/cmd/gnoland/start.go +++ b/gno.land/cmd/gnoland/start.go @@ -36,7 +36,6 @@ type startCfg struct { chainID string genesisRemote string dataDir string - genesisMaxVMCycles int64 config string txEventStoreType string @@ -125,13 +124,6 @@ func (c *startCfg) RegisterFlags(fs *flag.FlagSet) { "replacement for '%%REMOTE%%' in genesis", ) - fs.Int64Var( - &c.genesisMaxVMCycles, - "genesis-max-vm-cycles", - 10_000_000, - "set maximum allowed vm cycles per operation. Zero means no limit.", - ) - fs.StringVar( &c.config, flagConfigFlag, @@ -254,7 +246,7 @@ func execStart(c *startCfg, io commands.IO) error { cfg.TxEventStore = txEventStoreCfg // Create application and node. - gnoApp, err := gnoland.NewApp(dataDir, c.skipFailingGenesisTxs, logger, c.genesisMaxVMCycles) + gnoApp, err := gnoland.NewApp(dataDir, c.skipFailingGenesisTxs, logger) if err != nil { return fmt.Errorf("error in creating new app: %w", err) } diff --git a/gno.land/pkg/gnoland/app.go b/gno.land/pkg/gnoland/app.go index cc15f74134e..86fb6321386 100644 --- a/gno.land/pkg/gnoland/app.go +++ b/gno.land/pkg/gnoland/app.go @@ -31,7 +31,6 @@ type AppOptions struct { GnoRootDir string SkipFailingGenesisTxs bool Logger *slog.Logger - MaxCycles int64 } func NewAppOptions() *AppOptions { @@ -78,7 +77,7 @@ func NewAppWithOptions(cfg *AppOptions) (abci.Application, error) { // XXX: Embed this ? stdlibsDir := filepath.Join(cfg.GnoRootDir, "gnovm", "stdlibs") - vmKpr := vm.NewVMKeeper(baseKey, mainKey, acctKpr, bankKpr, stdlibsDir, cfg.MaxCycles) + vmKpr := vm.NewVMKeeper(baseKey, mainKey, acctKpr, bankKpr, stdlibsDir) // Set InitChainer baseApp.SetInitChainer(InitChainer(baseApp, acctKpr, bankKpr, cfg.SkipFailingGenesisTxs)) @@ -123,7 +122,7 @@ func NewAppWithOptions(cfg *AppOptions) (abci.Application, error) { } // NewApp creates the GnoLand application. -func NewApp(dataRootDir string, skipFailingGenesisTxs bool, logger *slog.Logger, maxCycles int64) (abci.Application, error) { +func NewApp(dataRootDir string, skipFailingGenesisTxs bool, logger *slog.Logger) (abci.Application, error) { var err error cfg := NewAppOptions() diff --git a/gno.land/pkg/gnoland/node_inmemory.go b/gno.land/pkg/gnoland/node_inmemory.go index 89f222738d0..d8dc3caa485 100644 --- a/gno.land/pkg/gnoland/node_inmemory.go +++ b/gno.land/pkg/gnoland/node_inmemory.go @@ -25,7 +25,6 @@ type InMemoryNodeConfig struct { Genesis *bft.GenesisDoc TMConfig *tmcfg.Config SkipFailingGenesisTxs bool - GenesisMaxVMCycles int64 } // NewMockedPrivValidator generate a new key @@ -79,10 +78,9 @@ func NewDefaultInMemoryNodeConfig(rootdir string) *InMemoryNodeConfig { } return &InMemoryNodeConfig{ - PrivValidator: pv, - TMConfig: tm, - Genesis: genesis, - GenesisMaxVMCycles: 10_000_000, + PrivValidator: pv, + TMConfig: tm, + Genesis: genesis, } } @@ -115,7 +113,6 @@ func NewInMemoryNode(logger *slog.Logger, cfg *InMemoryNodeConfig) (*node.Node, Logger: logger, GnoRootDir: cfg.TMConfig.RootDir, SkipFailingGenesisTxs: cfg.SkipFailingGenesisTxs, - MaxCycles: cfg.GenesisMaxVMCycles, DB: memdb.NewMemDB(), }) if err != nil { diff --git a/gno.land/pkg/sdk/vm/common_test.go b/gno.land/pkg/sdk/vm/common_test.go index b65757da403..ec14ae8515b 100644 --- a/gno.land/pkg/sdk/vm/common_test.go +++ b/gno.land/pkg/sdk/vm/common_test.go @@ -39,7 +39,7 @@ func setupTestEnv() testEnv { acck := authm.NewAccountKeeper(iavlCapKey, std.ProtoBaseAccount) bank := bankm.NewBankKeeper(acck) stdlibsDir := filepath.Join("..", "..", "..", "..", "gnovm", "stdlibs") - vmk := NewVMKeeper(baseCapKey, iavlCapKey, acck, bank, stdlibsDir, 10_000_000) + vmk := NewVMKeeper(baseCapKey, iavlCapKey, acck, bank, stdlibsDir) vmk.Initialize(ms.MultiCacheWrap()) diff --git a/gno.land/pkg/sdk/vm/keeper.go b/gno.land/pkg/sdk/vm/keeper.go index 67710be620c..2e61b02a56d 100644 --- a/gno.land/pkg/sdk/vm/keeper.go +++ b/gno.land/pkg/sdk/vm/keeper.go @@ -22,6 +22,7 @@ import ( const ( maxAllocTx = 500 * 1000 * 1000 maxAllocQuery = 1500 * 1000 * 1000 // higher limit for queries + maxCycles = 10_000_000 ) // vm.VMKeeperI defines a module interface that supports Gno @@ -55,7 +56,6 @@ func NewVMKeeper( acck auth.AccountKeeper, bank bank.BankKeeper, stdlibsDir string, - maxCycles int64, ) *VMKeeper { // TODO: create an Options struct to avoid too many constructor parameters vmk := &VMKeeper{ From 2866d9787ecbbb50f6aae821524be913cd2ff4db Mon Sep 17 00:00:00 2001 From: deelawn Date: Wed, 27 Mar 2024 16:17:48 -0700 Subject: [PATCH 02/14] Revert "don't allow max cycle configuration" This reverts commit e34a322859250dc4d440a0fa19a88d448112ab2f. --- gno.land/cmd/gnoland/start.go | 10 +++++++++- gno.land/pkg/gnoland/app.go | 5 +++-- gno.land/pkg/gnoland/node_inmemory.go | 9 ++++++--- gno.land/pkg/sdk/vm/common_test.go | 2 +- gno.land/pkg/sdk/vm/keeper.go | 2 +- 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/gno.land/cmd/gnoland/start.go b/gno.land/cmd/gnoland/start.go index 7fa7de32e5c..2b1757706f8 100644 --- a/gno.land/cmd/gnoland/start.go +++ b/gno.land/cmd/gnoland/start.go @@ -36,6 +36,7 @@ type startCfg struct { chainID string genesisRemote string dataDir string + genesisMaxVMCycles int64 config string txEventStoreType string @@ -124,6 +125,13 @@ func (c *startCfg) RegisterFlags(fs *flag.FlagSet) { "replacement for '%%REMOTE%%' in genesis", ) + fs.Int64Var( + &c.genesisMaxVMCycles, + "genesis-max-vm-cycles", + 10_000_000, + "set maximum allowed vm cycles per operation. Zero means no limit.", + ) + fs.StringVar( &c.config, flagConfigFlag, @@ -246,7 +254,7 @@ func execStart(c *startCfg, io commands.IO) error { cfg.TxEventStore = txEventStoreCfg // Create application and node. - gnoApp, err := gnoland.NewApp(dataDir, c.skipFailingGenesisTxs, logger) + gnoApp, err := gnoland.NewApp(dataDir, c.skipFailingGenesisTxs, logger, c.genesisMaxVMCycles) if err != nil { return fmt.Errorf("error in creating new app: %w", err) } diff --git a/gno.land/pkg/gnoland/app.go b/gno.land/pkg/gnoland/app.go index 86fb6321386..cc15f74134e 100644 --- a/gno.land/pkg/gnoland/app.go +++ b/gno.land/pkg/gnoland/app.go @@ -31,6 +31,7 @@ type AppOptions struct { GnoRootDir string SkipFailingGenesisTxs bool Logger *slog.Logger + MaxCycles int64 } func NewAppOptions() *AppOptions { @@ -77,7 +78,7 @@ func NewAppWithOptions(cfg *AppOptions) (abci.Application, error) { // XXX: Embed this ? stdlibsDir := filepath.Join(cfg.GnoRootDir, "gnovm", "stdlibs") - vmKpr := vm.NewVMKeeper(baseKey, mainKey, acctKpr, bankKpr, stdlibsDir) + vmKpr := vm.NewVMKeeper(baseKey, mainKey, acctKpr, bankKpr, stdlibsDir, cfg.MaxCycles) // Set InitChainer baseApp.SetInitChainer(InitChainer(baseApp, acctKpr, bankKpr, cfg.SkipFailingGenesisTxs)) @@ -122,7 +123,7 @@ func NewAppWithOptions(cfg *AppOptions) (abci.Application, error) { } // NewApp creates the GnoLand application. -func NewApp(dataRootDir string, skipFailingGenesisTxs bool, logger *slog.Logger) (abci.Application, error) { +func NewApp(dataRootDir string, skipFailingGenesisTxs bool, logger *slog.Logger, maxCycles int64) (abci.Application, error) { var err error cfg := NewAppOptions() diff --git a/gno.land/pkg/gnoland/node_inmemory.go b/gno.land/pkg/gnoland/node_inmemory.go index d8dc3caa485..89f222738d0 100644 --- a/gno.land/pkg/gnoland/node_inmemory.go +++ b/gno.land/pkg/gnoland/node_inmemory.go @@ -25,6 +25,7 @@ type InMemoryNodeConfig struct { Genesis *bft.GenesisDoc TMConfig *tmcfg.Config SkipFailingGenesisTxs bool + GenesisMaxVMCycles int64 } // NewMockedPrivValidator generate a new key @@ -78,9 +79,10 @@ func NewDefaultInMemoryNodeConfig(rootdir string) *InMemoryNodeConfig { } return &InMemoryNodeConfig{ - PrivValidator: pv, - TMConfig: tm, - Genesis: genesis, + PrivValidator: pv, + TMConfig: tm, + Genesis: genesis, + GenesisMaxVMCycles: 10_000_000, } } @@ -113,6 +115,7 @@ func NewInMemoryNode(logger *slog.Logger, cfg *InMemoryNodeConfig) (*node.Node, Logger: logger, GnoRootDir: cfg.TMConfig.RootDir, SkipFailingGenesisTxs: cfg.SkipFailingGenesisTxs, + MaxCycles: cfg.GenesisMaxVMCycles, DB: memdb.NewMemDB(), }) if err != nil { diff --git a/gno.land/pkg/sdk/vm/common_test.go b/gno.land/pkg/sdk/vm/common_test.go index ec14ae8515b..b65757da403 100644 --- a/gno.land/pkg/sdk/vm/common_test.go +++ b/gno.land/pkg/sdk/vm/common_test.go @@ -39,7 +39,7 @@ func setupTestEnv() testEnv { acck := authm.NewAccountKeeper(iavlCapKey, std.ProtoBaseAccount) bank := bankm.NewBankKeeper(acck) stdlibsDir := filepath.Join("..", "..", "..", "..", "gnovm", "stdlibs") - vmk := NewVMKeeper(baseCapKey, iavlCapKey, acck, bank, stdlibsDir) + vmk := NewVMKeeper(baseCapKey, iavlCapKey, acck, bank, stdlibsDir, 10_000_000) vmk.Initialize(ms.MultiCacheWrap()) diff --git a/gno.land/pkg/sdk/vm/keeper.go b/gno.land/pkg/sdk/vm/keeper.go index 2e61b02a56d..67710be620c 100644 --- a/gno.land/pkg/sdk/vm/keeper.go +++ b/gno.land/pkg/sdk/vm/keeper.go @@ -22,7 +22,6 @@ import ( const ( maxAllocTx = 500 * 1000 * 1000 maxAllocQuery = 1500 * 1000 * 1000 // higher limit for queries - maxCycles = 10_000_000 ) // vm.VMKeeperI defines a module interface that supports Gno @@ -56,6 +55,7 @@ func NewVMKeeper( acck auth.AccountKeeper, bank bank.BankKeeper, stdlibsDir string, + maxCycles int64, ) *VMKeeper { // TODO: create an Options struct to avoid too many constructor parameters vmk := &VMKeeper{ From 63a9b5b39da93b0543ae09b90f5189c2f7f6951d Mon Sep 17 00:00:00 2001 From: deelawn Date: Thu, 11 Apr 2024 11:43:43 -0700 Subject: [PATCH 03/14] another try solving star expr assignment issue --- gnovm/pkg/gnolang/op_assign.go | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/gnovm/pkg/gnolang/op_assign.go b/gnovm/pkg/gnolang/op_assign.go index 0cc30861355..38abecfb319 100644 --- a/gnovm/pkg/gnolang/op_assign.go +++ b/gnovm/pkg/gnolang/op_assign.go @@ -31,8 +31,27 @@ func (m *Machine) doOpAssign() { // forward order, not the usual reverse. rvs := m.PopValues(len(s.Lhs)) for i := len(s.Lhs) - 1; 0 <= i; i-- { + var lhsIsStarExpr bool + lhsExpr := s.Lhs[i] + if _, lhsIsStarExpr = lhsExpr.(*StarExpr); lhsIsStarExpr { + // We need to unwrap the lhs value to get to the pointer's base and index. + lhsExpr = lhsExpr.(*StarExpr).X + // We didn't know this star expression was the left hand side of an assingment, + // so we need to pop the value off the stack. + m.PopValue() + } + // Pop lhs value and desired type. - lv := m.PopAsPointer(s.Lhs[i]) + lv := m.PopAsPointer(lhsExpr) + if lhsIsStarExpr { + // Copy over the base and index so the pointer value gets updated in Assign2. + // These base and index values will not be persisted. + newLV := lv.TV.V.(PointerValue) + newLV.Base = lv.Base + newLV.Index = lv.Index + lv = newLV + } + // XXX HACK (until value persistence impl'd) if m.ReadOnly { if oo, ok := lv.Base.(Object); ok { From e59b442e3f921d6ac19d038860244052d0387717 Mon Sep 17 00:00:00 2001 From: deelawn Date: Tue, 16 Apr 2024 20:30:38 -0700 Subject: [PATCH 04/14] trying for a complete fix --- gnovm/pkg/gnolang/machine.go | 6 +++- gnovm/pkg/gnolang/nodes.go | 3 +- gnovm/pkg/gnolang/op_assign.go | 53 +++++++++++++++++++++++---------- gnovm/pkg/gnolang/transcribe.go | 3 ++ 4 files changed, 48 insertions(+), 17 deletions(-) diff --git a/gnovm/pkg/gnolang/machine.go b/gnovm/pkg/gnolang/machine.go index ea7be1d1f22..c07751eb719 100644 --- a/gnovm/pkg/gnolang/machine.go +++ b/gnovm/pkg/gnolang/machine.go @@ -1904,7 +1904,11 @@ func (m *Machine) PushForPointer(lx Expr) { m.PushOp(OpEval) case *StarExpr: // evaluate X (a reference) - m.PushExpr(lx.X) + if lx.IsLHS { + m.PushExpr(&RefExpr{X: lx.X}) + } else { + m.PushExpr(lx.X) + } m.PushOp(OpEval) case *CompositeLitExpr: // for *RefExpr e.g. &mystruct{} // evaluate lx. diff --git a/gnovm/pkg/gnolang/nodes.go b/gnovm/pkg/gnolang/nodes.go index 5d430a8bcde..cbac409a418 100644 --- a/gnovm/pkg/gnolang/nodes.go +++ b/gnovm/pkg/gnolang/nodes.go @@ -440,7 +440,8 @@ type SliceExpr struct { // X[Low:High:Max] // expression, or a pointer type. type StarExpr struct { // *X Attributes - X Expr // operand + IsLHS bool // if true, is left-hand side of assignment + X Expr // operand } type RefExpr struct { // &X diff --git a/gnovm/pkg/gnolang/op_assign.go b/gnovm/pkg/gnolang/op_assign.go index 38abecfb319..f449aeaee60 100644 --- a/gnovm/pkg/gnolang/op_assign.go +++ b/gnovm/pkg/gnolang/op_assign.go @@ -31,25 +31,48 @@ func (m *Machine) doOpAssign() { // forward order, not the usual reverse. rvs := m.PopValues(len(s.Lhs)) for i := len(s.Lhs) - 1; 0 <= i; i-- { - var lhsIsStarExpr bool lhsExpr := s.Lhs[i] - if _, lhsIsStarExpr = lhsExpr.(*StarExpr); lhsIsStarExpr { - // We need to unwrap the lhs value to get to the pointer's base and index. - lhsExpr = lhsExpr.(*StarExpr).X - // We didn't know this star expression was the left hand side of an assingment, - // so we need to pop the value off the stack. - m.PopValue() - } + _, lhsIsStarExpr := lhsExpr.(*StarExpr) + rv := rvs[i] // Pop lhs value and desired type. lv := m.PopAsPointer(lhsExpr) if lhsIsStarExpr { - // Copy over the base and index so the pointer value gets updated in Assign2. - // These base and index values will not be persisted. - newLV := lv.TV.V.(PointerValue) - newLV.Base = lv.Base - newLV.Index = lv.Index - lv = newLV + rvCopy := rv.Copy(m.Alloc) + + if lvPtrValue := lv.TV.V.(PointerValue); lvPtrValue.Base == nil { + // The LHS is a star expression and the pointer value it is referring to has no base. + // It must have been set in one two ways: + // 1. assigned to a value using the `new()` function + // 2. assigned to a literal pointer like &Type{} + // + // If there are any other references to this value, they can only be in the local scope, + // and any other references will be updated accordingly if the value is changed before + // this is persisted to the realm (if it is persisted to the realm. + rv = TypedValue{ + T: lv.TV.T, + V: PointerValue{ + TV: &rvCopy, + // Base: lv.TV.V.(PointerValue).Base, + // Index: lv.TV.V.(PointerValue).Index, + }, + } + } else { + // The LHS has an underlying value of a pointer with a base, so construct a new + // PointerValue for the RHS with a pointer to the existing LHS value and then overwrite + // that pointer's TypeValue with a copy of the RHS value. This ensures that the value of + // the LHS's base value is also updated. The only reason a new TypedValue is created here + // is because of how the `Assign2` method works. + rv = TypedValue{ + T: lv.TV.T, + V: PointerValue{ + TV: lv.TV.V.(PointerValue).TV, + Base: lv.TV.V.(PointerValue).Base, + Index: lv.TV.V.(PointerValue).Index, + }, + } + *rv.V.(PointerValue).TV = rvCopy + } } // XXX HACK (until value persistence impl'd) @@ -60,7 +83,7 @@ func (m *Machine) doOpAssign() { } } } - lv.Assign2(m.Alloc, m.Store, m.Realm, rvs[i], true) + lv.Assign2(m.Alloc, m.Store, m.Realm, rv, true) } } diff --git a/gnovm/pkg/gnolang/transcribe.go b/gnovm/pkg/gnolang/transcribe.go index c5b72336c83..d22cd6333ec 100644 --- a/gnovm/pkg/gnolang/transcribe.go +++ b/gnovm/pkg/gnolang/transcribe.go @@ -361,6 +361,9 @@ func transcribe(t Transform, ns []Node, ftype TransField, index int, n Node, nc case *AssignStmt: for idx := range cnn.Lhs { cnn.Lhs[idx] = transcribe(t, nns, TRANS_ASSIGN_LHS, idx, cnn.Lhs[idx], &c).(Expr) + if starExpr, ok := cnn.Lhs[idx].(*StarExpr); ok { + starExpr.IsLHS = true + } if isBreak(c) { break } else if isStopOrSkip(nc, c) { From abe008e6bfc3e8820ab9f066f5fd7d5886fbc118 Mon Sep 17 00:00:00 2001 From: deelawn Date: Mon, 22 Apr 2024 21:04:06 -0700 Subject: [PATCH 05/14] progress? --- gnovm/pkg/gnolang/op_assign.go | 44 +++++++--------------------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/gnovm/pkg/gnolang/op_assign.go b/gnovm/pkg/gnolang/op_assign.go index f449aeaee60..fa6e5a04f58 100644 --- a/gnovm/pkg/gnolang/op_assign.go +++ b/gnovm/pkg/gnolang/op_assign.go @@ -33,46 +33,20 @@ func (m *Machine) doOpAssign() { for i := len(s.Lhs) - 1; 0 <= i; i-- { lhsExpr := s.Lhs[i] _, lhsIsStarExpr := lhsExpr.(*StarExpr) - rv := rvs[i] // Pop lhs value and desired type. lv := m.PopAsPointer(lhsExpr) + + // A star expression on the lefthand side of an assign statement is a bit of a + // special case and needs to be handled. if lhsIsStarExpr { - rvCopy := rv.Copy(m.Alloc) - if lvPtrValue := lv.TV.V.(PointerValue); lvPtrValue.Base == nil { - // The LHS is a star expression and the pointer value it is referring to has no base. - // It must have been set in one two ways: - // 1. assigned to a value using the `new()` function - // 2. assigned to a literal pointer like &Type{} - // - // If there are any other references to this value, they can only be in the local scope, - // and any other references will be updated accordingly if the value is changed before - // this is persisted to the realm (if it is persisted to the realm. - rv = TypedValue{ - T: lv.TV.T, - V: PointerValue{ - TV: &rvCopy, - // Base: lv.TV.V.(PointerValue).Base, - // Index: lv.TV.V.(PointerValue).Index, - }, - } - } else { - // The LHS has an underlying value of a pointer with a base, so construct a new - // PointerValue for the RHS with a pointer to the existing LHS value and then overwrite - // that pointer's TypeValue with a copy of the RHS value. This ensures that the value of - // the LHS's base value is also updated. The only reason a new TypedValue is created here - // is because of how the `Assign2` method works. - rv = TypedValue{ - T: lv.TV.T, - V: PointerValue{ - TV: lv.TV.V.(PointerValue).TV, - Base: lv.TV.V.(PointerValue).Base, - Index: lv.TV.V.(PointerValue).Index, - }, - } - *rv.V.(PointerValue).TV = rvCopy + lvPtrValue := lv.TV.V.(PointerValue) + if lvPtrValue.Base == nil { + lvPtrValue.Base = lv.Base } + + lv = lvPtrValue } // XXX HACK (until value persistence impl'd) @@ -83,7 +57,7 @@ func (m *Machine) doOpAssign() { } } } - lv.Assign2(m.Alloc, m.Store, m.Realm, rv, true) + lv.Assign2(m.Alloc, m.Store, m.Realm, rvs[i], true) } } From 6ec50edb3ff2ce8bd66de0db58961c79d27deef9 Mon Sep 17 00:00:00 2001 From: deelawn Date: Mon, 22 Apr 2024 21:18:56 -0700 Subject: [PATCH 06/14] limit only to assign (temp) --- gnovm/pkg/gnolang/transcribe.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gnovm/pkg/gnolang/transcribe.go b/gnovm/pkg/gnolang/transcribe.go index d22cd6333ec..c9b6fb937c5 100644 --- a/gnovm/pkg/gnolang/transcribe.go +++ b/gnovm/pkg/gnolang/transcribe.go @@ -361,7 +361,7 @@ func transcribe(t Transform, ns []Node, ftype TransField, index int, n Node, nc case *AssignStmt: for idx := range cnn.Lhs { cnn.Lhs[idx] = transcribe(t, nns, TRANS_ASSIGN_LHS, idx, cnn.Lhs[idx], &c).(Expr) - if starExpr, ok := cnn.Lhs[idx].(*StarExpr); ok { + if starExpr, ok := cnn.Lhs[idx].(*StarExpr); ok && cnn.Op == ASSIGN { starExpr.IsLHS = true } if isBreak(c) { From a572d566d691b6d30ef90ffa4fd41335d26e23a1 Mon Sep 17 00:00:00 2001 From: deelawn Date: Mon, 22 Apr 2024 21:28:43 -0700 Subject: [PATCH 07/14] move starexpr logic to PopAsPointer --- gnovm/pkg/gnolang/machine.go | 11 +++++++++++ gnovm/pkg/gnolang/op_assign.go | 13 ------------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/gnovm/pkg/gnolang/machine.go b/gnovm/pkg/gnolang/machine.go index c07751eb719..cc30f375290 100644 --- a/gnovm/pkg/gnolang/machine.go +++ b/gnovm/pkg/gnolang/machine.go @@ -1935,6 +1935,17 @@ func (m *Machine) PopAsPointer(lx Expr) PointerValue { return xv.GetPointerTo(m.Alloc, m.Store, lx.Path) case *StarExpr: ptr := m.PopValue().V.(PointerValue) + if lx.IsLHS { + // A star expression on the lefthand side of an assign statement is a bit of a + // special case and needs to be handled. + innerPtr := ptr.TV.V.(PointerValue) + if innerPtr.Base == nil { + innerPtr.Base = ptr.Base + } + + ptr = innerPtr + } + return ptr case *CompositeLitExpr: // for *RefExpr tv := *m.PopValue() diff --git a/gnovm/pkg/gnolang/op_assign.go b/gnovm/pkg/gnolang/op_assign.go index fa6e5a04f58..19f2976e694 100644 --- a/gnovm/pkg/gnolang/op_assign.go +++ b/gnovm/pkg/gnolang/op_assign.go @@ -32,23 +32,10 @@ func (m *Machine) doOpAssign() { rvs := m.PopValues(len(s.Lhs)) for i := len(s.Lhs) - 1; 0 <= i; i-- { lhsExpr := s.Lhs[i] - _, lhsIsStarExpr := lhsExpr.(*StarExpr) // Pop lhs value and desired type. lv := m.PopAsPointer(lhsExpr) - // A star expression on the lefthand side of an assign statement is a bit of a - // special case and needs to be handled. - if lhsIsStarExpr { - - lvPtrValue := lv.TV.V.(PointerValue) - if lvPtrValue.Base == nil { - lvPtrValue.Base = lv.Base - } - - lv = lvPtrValue - } - // XXX HACK (until value persistence impl'd) if m.ReadOnly { if oo, ok := lv.Base.(Object); ok { From 601cc5e8c83f213f12619cf7fc47214782de8114 Mon Sep 17 00:00:00 2001 From: deelawn Date: Mon, 22 Apr 2024 21:29:26 -0700 Subject: [PATCH 08/14] removed op filter --- gnovm/pkg/gnolang/transcribe.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gnovm/pkg/gnolang/transcribe.go b/gnovm/pkg/gnolang/transcribe.go index c9b6fb937c5..d22cd6333ec 100644 --- a/gnovm/pkg/gnolang/transcribe.go +++ b/gnovm/pkg/gnolang/transcribe.go @@ -361,7 +361,7 @@ func transcribe(t Transform, ns []Node, ftype TransField, index int, n Node, nc case *AssignStmt: for idx := range cnn.Lhs { cnn.Lhs[idx] = transcribe(t, nns, TRANS_ASSIGN_LHS, idx, cnn.Lhs[idx], &c).(Expr) - if starExpr, ok := cnn.Lhs[idx].(*StarExpr); ok && cnn.Op == ASSIGN { + if starExpr, ok := cnn.Lhs[idx].(*StarExpr); ok { starExpr.IsLHS = true } if isBreak(c) { From 14b5f3c05b45de2d0d30ff9983a7aedd194c1914 Mon Sep 17 00:00:00 2001 From: deelawn Date: Mon, 22 Apr 2024 21:47:14 -0700 Subject: [PATCH 09/14] revert unnecessary change --- gnovm/pkg/gnolang/op_assign.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/gnovm/pkg/gnolang/op_assign.go b/gnovm/pkg/gnolang/op_assign.go index 19f2976e694..943d4895f85 100644 --- a/gnovm/pkg/gnolang/op_assign.go +++ b/gnovm/pkg/gnolang/op_assign.go @@ -31,10 +31,8 @@ func (m *Machine) doOpAssign() { // forward order, not the usual reverse. rvs := m.PopValues(len(s.Lhs)) for i := len(s.Lhs) - 1; 0 <= i; i-- { - lhsExpr := s.Lhs[i] - // Pop lhs value and desired type. - lv := m.PopAsPointer(lhsExpr) + lv := m.PopAsPointer(s.Lhs[i]) // XXX HACK (until value persistence impl'd) if m.ReadOnly { From 8e2381d287c7d20533675951aff3ceae485180d5 Mon Sep 17 00:00:00 2001 From: deelawn Date: Tue, 23 Apr 2024 07:35:11 -0700 Subject: [PATCH 10/14] move where IsLHS is set --- gnovm/pkg/gnolang/transcribe.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/gnovm/pkg/gnolang/transcribe.go b/gnovm/pkg/gnolang/transcribe.go index d22cd6333ec..2cfeeac285d 100644 --- a/gnovm/pkg/gnolang/transcribe.go +++ b/gnovm/pkg/gnolang/transcribe.go @@ -210,6 +210,7 @@ func transcribe(t Transform, ns []Node, ftype TransField, index int, n Node, nc } case *StarExpr: cnn.X = transcribe(t, nns, TRANS_STAR_X, 0, cnn.X, &c).(Expr) + cnn.IsLHS = ftype == TRANS_ASSIGN_LHS if isStopOrSkip(nc, c) { return } @@ -361,9 +362,6 @@ func transcribe(t Transform, ns []Node, ftype TransField, index int, n Node, nc case *AssignStmt: for idx := range cnn.Lhs { cnn.Lhs[idx] = transcribe(t, nns, TRANS_ASSIGN_LHS, idx, cnn.Lhs[idx], &c).(Expr) - if starExpr, ok := cnn.Lhs[idx].(*StarExpr); ok { - starExpr.IsLHS = true - } if isBreak(c) { break } else if isStopOrSkip(nc, c) { From 1385b71451fc82f368c5ef55f76d746db0141728 Mon Sep 17 00:00:00 2001 From: deelawn Date: Tue, 23 Apr 2024 07:35:33 -0700 Subject: [PATCH 11/14] comments, formatting --- gnovm/pkg/gnolang/machine.go | 12 +++++++++++- gnovm/pkg/gnolang/op_assign.go | 1 - 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/gnovm/pkg/gnolang/machine.go b/gnovm/pkg/gnolang/machine.go index cc30f375290..eabb1f8fbb3 100644 --- a/gnovm/pkg/gnolang/machine.go +++ b/gnovm/pkg/gnolang/machine.go @@ -1905,6 +1905,12 @@ func (m *Machine) PushForPointer(lx Expr) { case *StarExpr: // evaluate X (a reference) if lx.IsLHS { + // If the star expression is on the LHS, evaluate the expression as + // a reference. This ensures the value that is pushed is a pointer to + // the pointer value represented by the lx.X expression. This will be + // helpful if the underlying pointer value dos not have a base; + // the base of the reference to the pointer value can be used instead, + // to properly mark the owning object as dirty. m.PushExpr(&RefExpr{X: lx.X}) } else { m.PushExpr(lx.X) @@ -1937,7 +1943,11 @@ func (m *Machine) PopAsPointer(lx Expr) PointerValue { ptr := m.PopValue().V.(PointerValue) if lx.IsLHS { // A star expression on the lefthand side of an assign statement is a bit of a - // special case and needs to be handled. + // special case and needs to be handled. The value pushed for it is always a + // reference to the actual value, so first dereference it by assigning it to + // innerPtr. If the pointer value has no base, use the base of its parent. + // Assigning a non-nil base ensures the parent gets marked as dirty and the + // updated value is persisted during realm finalization. innerPtr := ptr.TV.V.(PointerValue) if innerPtr.Base == nil { innerPtr.Base = ptr.Base diff --git a/gnovm/pkg/gnolang/op_assign.go b/gnovm/pkg/gnolang/op_assign.go index 943d4895f85..0cc30861355 100644 --- a/gnovm/pkg/gnolang/op_assign.go +++ b/gnovm/pkg/gnolang/op_assign.go @@ -33,7 +33,6 @@ func (m *Machine) doOpAssign() { for i := len(s.Lhs) - 1; 0 <= i; i-- { // Pop lhs value and desired type. lv := m.PopAsPointer(s.Lhs[i]) - // XXX HACK (until value persistence impl'd) if m.ReadOnly { if oo, ok := lv.Base.(Object); ok { From 4f21da8f6145760e72d59e53c14bc6f0f4ec4f0c Mon Sep 17 00:00:00 2001 From: deelawn Date: Tue, 23 Apr 2024 08:04:38 -0700 Subject: [PATCH 12/14] better variable name --- gnovm/pkg/gnolang/machine.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gnovm/pkg/gnolang/machine.go b/gnovm/pkg/gnolang/machine.go index eabb1f8fbb3..32c56f3462f 100644 --- a/gnovm/pkg/gnolang/machine.go +++ b/gnovm/pkg/gnolang/machine.go @@ -1945,15 +1945,15 @@ func (m *Machine) PopAsPointer(lx Expr) PointerValue { // A star expression on the lefthand side of an assign statement is a bit of a // special case and needs to be handled. The value pushed for it is always a // reference to the actual value, so first dereference it by assigning it to - // innerPtr. If the pointer value has no base, use the base of its parent. + // valuePtr. If the pointer value has no base, use the base of its parent. // Assigning a non-nil base ensures the parent gets marked as dirty and the // updated value is persisted during realm finalization. - innerPtr := ptr.TV.V.(PointerValue) - if innerPtr.Base == nil { - innerPtr.Base = ptr.Base + valuePtr := ptr.TV.V.(PointerValue) + if valuePtr.Base == nil { + valuePtr.Base = ptr.Base } - ptr = innerPtr + ptr = valuePtr } return ptr From d1b1ef5528bc88cbb70d35ecefc5b6fb204bb0a3 Mon Sep 17 00:00:00 2001 From: deelawn Date: Tue, 23 Apr 2024 08:30:58 -0700 Subject: [PATCH 13/14] added tests --- .../cmd/gnoland/testdata/issue-1326.txtar | 68 +++++++++++++++++++ .../cmd/gnoland/testdata/star-assign.txtar | 61 +++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 gno.land/cmd/gnoland/testdata/issue-1326.txtar create mode 100644 gno.land/cmd/gnoland/testdata/star-assign.txtar diff --git a/gno.land/cmd/gnoland/testdata/issue-1326.txtar b/gno.land/cmd/gnoland/testdata/issue-1326.txtar new file mode 100644 index 00000000000..2b9ba952446 --- /dev/null +++ b/gno.land/cmd/gnoland/testdata/issue-1326.txtar @@ -0,0 +1,68 @@ +gnoland start + +# add contract +gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/demo/xx -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 +stdout OK! + +# execute New +gnokey maketx call -pkgpath gno.land/r/demo/xx -func New -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 +stdout OK! + +# execute Delta for the first time +gnokey maketx call -pkgpath gno.land/r/demo/xx -func Delta -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 +stdout '("1,1,1;" string)' +stdout OK! + +gnokey maketx call -pkgpath gno.land/r/demo/xx -func Delta -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 +stdout '("1,1,1;2,2,2;" string)' +stdout OK! + +-- realm.gno -- +package xx + +import ( + "strconv" +) + +type Move struct { + N1, N2, N3 byte +} + +type S struct { + Moves []Move +} + +func (s S) clone() S { + mv := s.Moves + return S{Moves: mv} +} + +func (olds S) change() S { + s := olds.clone() + + counter++ + s.Moves = append([]Move{}, s.Moves...) + s.Moves = append(s.Moves, Move{counter, counter, counter}) + return s +} + +var el *S +var counter byte + +func New() { + el = new(S) +} + +func Delta() string { + n := el.change() + *el = n + return Values() +} + +func Values() string { + s := "" + for _, val := range el.Moves { + s += strconv.Itoa(int(val.N1)) + "," + strconv.Itoa(int(val.N2)) + "," + strconv.Itoa(int(val.N3)) + ";" + } + return s +} diff --git a/gno.land/cmd/gnoland/testdata/star-assign.txtar b/gno.land/cmd/gnoland/testdata/star-assign.txtar new file mode 100644 index 00000000000..1c8e2eea05c --- /dev/null +++ b/gno.land/cmd/gnoland/testdata/star-assign.txtar @@ -0,0 +1,61 @@ +loadpkg gno.land/p/demo/ufmt + +gnoland start + +# add contract +gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/demo/xx -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 +stdout OK! + +# execute New +gnokey maketx call -pkgpath gno.land/r/demo/xx -func New -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 +stdout OK! + +gnokey maketx call -pkgpath gno.land/r/demo/xx -func Delta -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 +stdout OK! + +gnokey maketx call -pkgpath gno.land/r/demo/xx -func Values -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1 +stdout '("1, hello, 100, 100, 8, 5, 8" string)' +stdout OK! + +-- realm.gno -- +package xx + +import ( + "gno.land/p/demo/ufmt" +) + +type A struct { + nums []int +} + +var ( + intPtr *int + strPtr *string + aPtr *A + concretePtr *int + concreteValue int +) + +func New() { + intPtr = new(int) + strPtr = new(string) + aPtr = &A{} + concretePtr = &concreteValue +} + +func Delta() { + *intPtr++ + *strPtr += "hello" + *aPtr = A{nums: []int{8, 5, 8}} + *concretePtr = 100 +} + +func Values() string { + var results string + results += ufmt.Sprintf("%d, %s, %d, %d", *intPtr, *strPtr, *concretePtr, concreteValue) + for _, n := range aPtr.nums { + results += ufmt.Sprintf(", %d", n) + } + + return results +} From 9fc480057c4373177c9fdf04883b0657762d5982 Mon Sep 17 00:00:00 2001 From: deelawn Date: Fri, 31 May 2024 13:04:59 -0700 Subject: [PATCH 14/14] removed isLHS --- gnovm/pkg/gnolang/machine.go | 42 +++++++++++++++------------------ gnovm/pkg/gnolang/nodes.go | 3 +-- gnovm/pkg/gnolang/transcribe.go | 1 - 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/gnovm/pkg/gnolang/machine.go b/gnovm/pkg/gnolang/machine.go index 32c56f3462f..c72abcf45bb 100644 --- a/gnovm/pkg/gnolang/machine.go +++ b/gnovm/pkg/gnolang/machine.go @@ -1904,17 +1904,14 @@ func (m *Machine) PushForPointer(lx Expr) { m.PushOp(OpEval) case *StarExpr: // evaluate X (a reference) - if lx.IsLHS { - // If the star expression is on the LHS, evaluate the expression as - // a reference. This ensures the value that is pushed is a pointer to - // the pointer value represented by the lx.X expression. This will be - // helpful if the underlying pointer value dos not have a base; - // the base of the reference to the pointer value can be used instead, - // to properly mark the owning object as dirty. - m.PushExpr(&RefExpr{X: lx.X}) - } else { - m.PushExpr(lx.X) - } + + // The star expression is on the LHS, so evaluate the expression as + // a reference. This ensures the value that is pushed is a pointer to + // the pointer value represented by the lx.X expression. This will be + // helpful if the underlying pointer value dos not have a base; + // the base of the reference to the pointer value can be used instead, + // to properly mark the owning object as dirty. + m.PushExpr(&RefExpr{X: lx.X}) m.PushOp(OpEval) case *CompositeLitExpr: // for *RefExpr e.g. &mystruct{} // evaluate lx. @@ -1941,21 +1938,20 @@ func (m *Machine) PopAsPointer(lx Expr) PointerValue { return xv.GetPointerTo(m.Alloc, m.Store, lx.Path) case *StarExpr: ptr := m.PopValue().V.(PointerValue) - if lx.IsLHS { - // A star expression on the lefthand side of an assign statement is a bit of a - // special case and needs to be handled. The value pushed for it is always a - // reference to the actual value, so first dereference it by assigning it to - // valuePtr. If the pointer value has no base, use the base of its parent. - // Assigning a non-nil base ensures the parent gets marked as dirty and the - // updated value is persisted during realm finalization. - valuePtr := ptr.TV.V.(PointerValue) - if valuePtr.Base == nil { - valuePtr.Base = ptr.Base - } - ptr = valuePtr + // A star expression on the lefthand side of an assign statement is a bit of a + // special case and needs to be handled. The value pushed for it is always a + // reference to the actual value, so first dereference it by assigning it to + // valuePtr. If the pointer value has no base, use the base of its parent. + // Assigning a non-nil base ensures the parent gets marked as dirty and the + // updated value is persisted during realm finalization. + valuePtr := ptr.TV.V.(PointerValue) + if valuePtr.Base == nil { + valuePtr.Base = ptr.Base } + ptr = valuePtr + return ptr case *CompositeLitExpr: // for *RefExpr tv := *m.PopValue() diff --git a/gnovm/pkg/gnolang/nodes.go b/gnovm/pkg/gnolang/nodes.go index cbac409a418..5d430a8bcde 100644 --- a/gnovm/pkg/gnolang/nodes.go +++ b/gnovm/pkg/gnolang/nodes.go @@ -440,8 +440,7 @@ type SliceExpr struct { // X[Low:High:Max] // expression, or a pointer type. type StarExpr struct { // *X Attributes - IsLHS bool // if true, is left-hand side of assignment - X Expr // operand + X Expr // operand } type RefExpr struct { // &X diff --git a/gnovm/pkg/gnolang/transcribe.go b/gnovm/pkg/gnolang/transcribe.go index 2cfeeac285d..c5b72336c83 100644 --- a/gnovm/pkg/gnolang/transcribe.go +++ b/gnovm/pkg/gnolang/transcribe.go @@ -210,7 +210,6 @@ func transcribe(t Transform, ns []Node, ftype TransField, index int, n Node, nc } case *StarExpr: cnn.X = transcribe(t, nns, TRANS_STAR_X, 0, cnn.X, &c).(Expr) - cnn.IsLHS = ftype == TRANS_ASSIGN_LHS if isStopOrSkip(nc, c) { return }