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

WIP fix: handle assignments to dereferenced pointer values #1501

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
75 changes: 75 additions & 0 deletions gno.land/cmd/gnoland/testdata/unexpected-unreal.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# Reproducible Test for https://github.com/gnolang/gno/issues/1167
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad you tried txtar-based tests. How was your experience? Do you have any suggestions for improvement? If yes, please refer to #1269.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't me, I just copied this: https://gist.github.com/thehowl/e36b0f0d652a2a348a2fcd331a310417. Added a comment there. We should avoid using environment variables for production. Other than that I'm liking it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My main thing with tests is this:

I just want "make test" to work as expected, at the root, and at the individual project levels. At the root it should just pass any options and call make test individually.

Anyone who pulls the codebase should be able to get make test passing, and go test ./... should all work as expected too.

Copy link
Member

Choose a reason for hiding this comment

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

My main thing with tests is this:

I just want "make test" to work as expected, at the root, and at the individual project levels. At the root it should just pass any options and call make test individually.

Anyone who pulls the codebase should be able to get make test passing, and go test ./... should all work as expected too.

That is the case!


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 OK!

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

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

-- 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
}

4 changes: 2 additions & 2 deletions gnovm/pkg/gnolang/op_call.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ func (m *Machine) doOpCall() {
// Make a copy so that a reference to the arguemnt isn't used
// in cases where the non-primitive value type is represented
// as a pointer, *StructValue, for example.
b.Values[i] = pv.Copy(m.Alloc)
b.Values[i] = pv.Copy(m.Alloc, m.Store)
}
}

Expand Down Expand Up @@ -400,7 +400,7 @@ func (m *Machine) doOpDefer() {

func (m *Machine) doOpPanic1() {
// Pop exception
var ex TypedValue = m.PopValue().Copy(m.Alloc)
var ex TypedValue = m.PopValue().Copy(m.Alloc, m.Store)
// Panic
m.Panic(ex)
}
Expand Down
16 changes: 8 additions & 8 deletions gnovm/pkg/gnolang/op_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@
*xv = *dv
} else {
dv = xv
*xv = xv.Copy(m.Alloc)
*xv = xv.Copy(m.Alloc, m.Store)
}
ll = dv.GetLength()
if ll == 0 { // early termination
Expand All @@ -173,7 +173,7 @@
case DEFINE:
knxp := bs.Key.(*NameExpr).Path
ptr := m.LastBlock().GetPointerTo(m.Store, knxp)
ptr.TV.Assign(m.Alloc, iv, false)
ptr.TV.Assign(m.Alloc, m.Store, iv, false)
default:
panic("should not happen")
}
Expand All @@ -188,7 +188,7 @@
case DEFINE:
vnxp := bs.Value.(*NameExpr).Path
ptr := m.LastBlock().GetPointerTo(m.Store, vnxp)
ptr.TV.Assign(m.Alloc, ev, false)
ptr.TV.Assign(m.Alloc, m.Store, ev, false)
default:
panic("should not happen")
}
Expand Down Expand Up @@ -269,7 +269,7 @@
case DEFINE:
knxp := bs.Key.(*NameExpr).Path
ptr := m.LastBlock().GetPointerTo(m.Store, knxp)
ptr.TV.Assign(m.Alloc, iv, false)
ptr.TV.Assign(m.Alloc, m.Store, iv, false)
default:
panic("should not happen")
}
Expand All @@ -282,7 +282,7 @@
case DEFINE:
vnxp := bs.Value.(*NameExpr).Path
ptr := m.LastBlock().GetPointerTo(m.Store, vnxp)
ptr.TV.Assign(m.Alloc, ev, false)
ptr.TV.Assign(m.Alloc, m.Store, ev, false)
default:
panic("should not happen")
}
Expand Down Expand Up @@ -362,7 +362,7 @@
case DEFINE:
knxp := bs.Key.(*NameExpr).Path
ptr := m.LastBlock().GetPointerTo(m.Store, knxp)
ptr.TV.Assign(m.Alloc, kv, false)
ptr.TV.Assign(m.Alloc, m.Store, kv, false)

Check warning on line 365 in gnovm/pkg/gnolang/op_exec.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/op_exec.go#L365

Added line #L365 was not covered by tests
default:
panic("should not happen")
}
Expand All @@ -375,7 +375,7 @@
case DEFINE:
vnxp := bs.Value.(*NameExpr).Path
ptr := m.LastBlock().GetPointerTo(m.Store, vnxp)
ptr.TV.Assign(m.Alloc, vv, false)
ptr.TV.Assign(m.Alloc, m.Store, vv, false)

Check warning on line 378 in gnovm/pkg/gnolang/op_exec.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/op_exec.go#L378

Added line #L378 was not covered by tests
default:
panic("should not happen")
}
Expand Down Expand Up @@ -885,7 +885,7 @@
vp := NewValuePath(
VPBlock, 1, 0, ss.VarName)
ptr := b.GetPointerTo(m.Store, vp)
ptr.TV.Assign(m.Alloc, *xv, false)
ptr.TV.Assign(m.Alloc, m.Store, *xv, false)

Check warning on line 888 in gnovm/pkg/gnolang/op_exec.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/op_exec.go#L888

Added line #L888 was not covered by tests
}
// expand block size
if nn := cs.GetNumNames(); int(nn) > len(b.Values) {
Expand Down
17 changes: 17 additions & 0 deletions gnovm/pkg/gnolang/ownership.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
IncRefCount() int
DecRefCount() int
GetRefCount() int
SetRefCount(int)
GetIsDirty() bool
SetIsDirty(bool, uint64)
GetIsEscaped() bool
Expand All @@ -117,6 +118,8 @@
GetIsNewDeleted() bool
SetIsNewDeleted(bool)
GetIsTransient() bool
GetNextObjectID() ObjectID
SetNextObjectID(ObjectID)

// Saves to realm along the way if owned, and also (dirty
// or new).
Expand Down Expand Up @@ -144,6 +147,7 @@
isNewReal bool
isNewEscaped bool
isNewDeleted bool
nextID ObjectID // set if replacing pre-existing object.

// XXX huh?
owner Object // mem reference to owner.
Expand All @@ -164,6 +168,7 @@
isNewReal: oi.isNewReal,
isNewEscaped: oi.isNewEscaped,
isNewDeleted: oi.isNewDeleted,
nextID: oi.nextID,
}
}

Expand Down Expand Up @@ -265,6 +270,10 @@
return oi.RefCount
}

func (oi *ObjectInfo) SetRefCount(rc int) {
oi.RefCount = rc

Check warning on line 274 in gnovm/pkg/gnolang/ownership.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/ownership.go#L273-L274

Added lines #L273 - L274 were not covered by tests
}

func (oi *ObjectInfo) GetIsDirty() bool {
return oi.isDirty
}
Expand Down Expand Up @@ -324,6 +333,14 @@
return false
}

func (oi *ObjectInfo) GetNextObjectID() ObjectID {
return oi.nextID
}

func (oi *ObjectInfo) SetNextObjectID(nid ObjectID) {
oi.nextID = nid

Check warning on line 341 in gnovm/pkg/gnolang/ownership.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/ownership.go#L340-L341

Added lines #L340 - L341 were not covered by tests
}

func (tv *TypedValue) GetFirstObject(store Store) Object {
switch cv := tv.V.(type) {
case PointerValue:
Expand Down
17 changes: 15 additions & 2 deletions gnovm/pkg/gnolang/realm.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,21 @@
if !oo.GetObjectID().IsZero() {
return
}
rlm.assignNewObjectID(oo)
rlm.created = append(rlm.created, oo)
noid := oo.GetNextObjectID()
if !noid.IsZero() {
// consume NextObjectID.
oo.SetObjectID(noid)
oo.SetNextObjectID(ObjectID{})
// rlm.created not touched, not actually.
// instead, is considered updated.
rlm.updated = append(rlm.updated, oo)
// however, we do need the following
// recurse logic to inc refcount
// children of this replacement object.

Check warning on line 403 in gnovm/pkg/gnolang/realm.go

View check run for this annotation

Codecov / codecov/patch

gnovm/pkg/gnolang/realm.go#L395-L403

Added lines #L395 - L403 were not covered by tests
} else {
rlm.assignNewObjectID(oo)
rlm.created = append(rlm.created, oo)
}
// RECURSE GUARD END

// recurse for children.
Expand Down
2 changes: 0 additions & 2 deletions gnovm/pkg/gnolang/uverse.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,6 @@ func UverseNode() *PackageNode {
if argsb.Data == nil {
for i := 0; i < argsl; i++ {
oldElem := list[xvo+xvl+i]
// unrefCopy will resolve references and copy their values
// to copy by value rather than by reference.
newElem := argsb.List[argso+i].unrefCopy(m.Alloc, m.Store)
list[xvo+xvl+i] = newElem

Expand Down
Loading
Loading