Skip to content
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

fix: handle assignments to dereferenced pointer values #1398

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
2c023cf
recurse on DidUpdate for all ArrayValue elements
deelawn Oct 26, 2023
c4ed7e4
undid incorrect simplification
deelawn Oct 26, 2023
09df263
reworked solution
deelawn Oct 27, 2023
cec8f0d
Added txtar test
deelawn Oct 27, 2023
7ecb905
Merge branch 'master' into bug/slice-append
deelawn Oct 27, 2023
91b4586
alternate approach to saving nested reference values
deelawn Oct 31, 2023
b2e153a
scoped back which types of values can be saved dynamically
deelawn Oct 31, 2023
4b4b25c
Revert "alternate approach to saving nested reference values"
deelawn Nov 1, 2023
7df8c80
handle nested slices when appending to slices under capacity
deelawn Nov 2, 2023
77f79a8
Merge branch 'master' into bug/slice-append
deelawn Nov 7, 2023
6821083
wrap new sanity check in debug
deelawn Nov 7, 2023
811b5f6
Merge branch 'master' into bug/slice-append
deelawn Nov 8, 2023
b4279e9
fixed up txtar test
deelawn Nov 8, 2023
daa6914
added XX annotation
deelawn Nov 8, 2023
750ec38
reworked solution to avoid looping in DidUpdate
deelawn Nov 14, 2023
7392890
txtar test for 1167 test case
deelawn Nov 14, 2023
9c0c35a
improved append test and added more append deep copies
deelawn Nov 15, 2023
de2a233
handle additional append cases
deelawn Nov 15, 2023
1d7497c
first stab at a solution that fixes the issue
deelawn Nov 29, 2023
9133293
added realm qualifier
deelawn Nov 29, 2023
ed2ad73
use object accessor method
deelawn Nov 29, 2023
e7589fe
add txtar; fix pkg path in other
deelawn Dec 7, 2023
bb9fb88
Merge branch 'master' into bug/ptr-deref-assgn
deelawn Jan 8, 2024
d03eec7
restore object info copy functionality
deelawn Jan 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 6 additions & 13 deletions gno.land/cmd/gnoland/testdata/issue-1167.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -3,40 +3,33 @@
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
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/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 -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
gnokey maketx call -pkgpath gno.land/r/xx -func New -args X -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 -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
gnokey maketx call -pkgpath gno.land/r/xx -func Delta -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '"1,1,1;" string'

# execute Delta for the second time
gnokey maketx call -pkgpath gno.land/r/demo/xx -func Delta -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
gnokey maketx call -pkgpath gno.land/r/xx -func Delta -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '1,1,1;2,2,2;" string'

# execute Delta for the third time
gnokey maketx call -pkgpath gno.land/r/demo/xx -func Delta -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
gnokey maketx call -pkgpath gno.land/r/xx -func Delta -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '1,1,1;2,2,2;3,3,3;" string'

# execute Render
gnokey maketx call -pkgpath gno.land/r/demo/xx -func Render -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
gnokey maketx call -pkgpath gno.land/r/xx -func Render -args X -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '1,1,1;2,2,2;3,3,3;" string'

-- gno.mod --
module gno.land/r/demo/xx

require (
gno.land/p/demo/avl v0.0.0-latest
)

-- realm.gno --
package xx

Expand Down
67 changes: 67 additions & 0 deletions gno.land/cmd/gnoland/testdata/issue-1326.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# Reproducible Test for https://github.com/gnolang/gno/issues/1167

gnoland start

# add contract
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/xx -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# execute New
gnokey maketx call -pkgpath gno.land/r/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/xx -func Delta -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

-- realm.gno --
package xx

import (
"strconv"

"gno.land/p/demo/avl"
)

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 = &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
}
50 changes: 47 additions & 3 deletions gnovm/pkg/gnolang/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,35 @@
// Special case of DataByte into (base=*SliceValue).Data.
pv.TV.SetDataByte(tv2.GetUint8())
return
} else if rlm != nil && pv.Base == nil && pv.Index == 0 {
// An index of zero indicates this is an assignment to a realm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An index of zero could mean it's the zero'th element of an array, or the zero'th element of a block,
or whatever the .Base type is. The Index doesn't say much but the .Base type says more but depends on what you mean by a realm object. Like a PackageValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that an index of zero can mean more than one thing depending on the context, but in this case the realm is not nil and the base is nil, so I think this means that a value is being assigned to a realm object. I'm not sure what the difference is between realm object and PackageValue; they might be synonymous -- the values that are persisted from one block to the next; variables that are declared in a realm and are not local to any function or method.

Copy link
Contributor

@jaekwon jaekwon Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok at least the comment needs fixing, "A base of nil indicates"?
A base of nil indicates an assignment to an escaped object, or a new one that just hasn't been persisted yet.
An escaped object has no owner.
All persisted objects are realm objects.
Some are substructures (base == parent, ref==1), others are escaped.
All realms have an associated package value but not all packages are realms. Depends on the PkgPath name.

// object. The base will be nil if this is being assigned to
// a dereferenced pointer value.

// First resolve the reference value and copy the underlying object
// info along with it.

// TODO: Should this check that tv2 type is not a reference?
if _, ok := pv.TV.V.(RefValue); ok {
*pv.TV = pv.TV.unrefCopySetRefObjectInfo(alloc, store)
}

Check warning on line 288 in gnovm/pkg/gnolang/values.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/values.go#L287-L288

Added lines #L287 - L288 were not covered by tests

// We need the owner to know which parent object to mark as dirty.
// Otherwise the change will never be persisted.
obj := pv.TV.GetFirstObject(store)
pv.Base = obj.GetOwner()
if pv.Base == nil {
// Can't use GetOwnerID() if GetOwner() returns nil. Not sure why this
// returns nil, the the object ID is likely there. Use that to get the object.
objInfo := obj.GetObjectInfo()
if !objInfo.OwnerID.IsZero() {
// Finally use this as the base so DidUpdate is called and all objects
// are correctly persisted to realm storage.
pv.Base = store.GetObject(objInfo.OwnerID)
}

Check warning on line 302 in gnovm/pkg/gnolang/values.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/values.go#L299-L302

Added lines #L299 - L302 were not covered by tests
}
}

// General case
if rlm != nil && pv.Base != nil {
oo1 := pv.TV.GetFirstObject(store)
Expand Down Expand Up @@ -1029,16 +1057,32 @@

// unrefCopy makes a copy of the underlying value in the case of reference values.
// It copies other values as expected using the normal Copy method.
func (tv TypedValue) unrefCopy(alloc *Allocator, store Store) (cp TypedValue) {
func (tv TypedValue) unrefCopy(alloc *Allocator, store Store) TypedValue {
return tv.unrefCopyCore(alloc, store, false)
}

func (tv TypedValue) unrefCopySetRefObjectInfo(alloc *Allocator, store Store) TypedValue {
return tv.unrefCopyCore(alloc, store, true)

Check warning on line 1065 in gnovm/pkg/gnolang/values.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/values.go#L1064-L1065

Added lines #L1064 - L1065 were not covered by tests
}

func (tv TypedValue) unrefCopyCore(alloc *Allocator, store Store, setRefObjectInfo bool) (cp TypedValue) {
switch tv.V.(type) {
case RefValue:
cp.T = tv.T
refObject := tv.GetFirstObject(store)
switch refObjectValue := refObject.(type) {
case *ArrayValue:
cp.V = refObjectValue.Copy(alloc)
arrayValue := refObjectValue.Copy(alloc)
if setRefObjectInfo {
arrayValue.ObjectInfo = *refObject.GetObjectInfo()
}
cp.V = arrayValue

Check warning on line 1079 in gnovm/pkg/gnolang/values.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/values.go#L1075-L1079

Added lines #L1075 - L1079 were not covered by tests
case *StructValue:
cp.V = refObjectValue.Copy(alloc)
structValue := refObjectValue.Copy(alloc)
if setRefObjectInfo {
structValue.ObjectInfo = *refObject.GetObjectInfo()
}
cp.V = structValue

Check warning on line 1085 in gnovm/pkg/gnolang/values.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/values.go#L1081-L1085

Added lines #L1081 - L1085 were not covered by tests
default:
cp = tv
}
Expand Down
Loading