Skip to content

Commit

Permalink
fix(gnovm): source location always precise with line and column (gnol…
Browse files Browse the repository at this point in the history
…ang#2362)

The column number was not stored in the GnoVM location. A `Nonce` field
was used to distinguished 2 block nodes on the same line. This was
applied only to block nodes, not expressions or statements.

As a result, the node location was the same for all expressions and
statements located at the same source line. This change ensures the
uniqueness of location for all nodes, in addition to providing an
accurate position in source code.

We replace `Nonce` by `Column`, computed by the Go tokenizer (the same
as line numbers). Column is always included in all AST nodes, and the
location string has a uniform format.

With this change it is now possible for the debugger to step in infinite
loops on one line like:

`for { i++; println(i) }`

This change is also necessary for a correct accounting for code
coverage, and also may be gas accounting.

Note: as `Nonce` is renamed to `Column` in protocol definitions (type
and index unchanged),
it may qualify as a BREAKING CHANGE, but I'm not completely sure of
that.
<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [*] Added new tests, or not needed, or not feasible
- [*] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [*] Updated the official documentation or not needed
- [*] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [*] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
  • Loading branch information
mvertes authored and gfanton committed Jul 23, 2024
1 parent 260db9d commit 107d4fc
Show file tree
Hide file tree
Showing 226 changed files with 331 additions and 313 deletions.
6 changes: 3 additions & 3 deletions examples/gno.land/p/demo/avl/z_0_filetest.gno
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "0",
// "File": "",
// "Line": "0",
// "Nonce": "0",
// "PkgPath": "gno.land/r/test"
// }
// },
Expand Down Expand Up @@ -258,9 +258,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "1",
// "File": "main.gno",
// "Line": "10",
// "Nonce": "0",
// "PkgPath": "gno.land/r/test"
// }
// },
Expand Down Expand Up @@ -294,9 +294,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "1",
// "File": "main.gno",
// "Line": "15",
// "Nonce": "0",
// "PkgPath": "gno.land/r/test"
// }
// },
Expand Down
6 changes: 3 additions & 3 deletions examples/gno.land/p/demo/avl/z_1_filetest.gno
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "0",
// "File": "",
// "Line": "0",
// "Nonce": "0",
// "PkgPath": "gno.land/r/test"
// }
// },
Expand Down Expand Up @@ -331,9 +331,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "1",
// "File": "main.gno",
// "Line": "10",
// "Nonce": "0",
// "PkgPath": "gno.land/r/test"
// }
// },
Expand Down Expand Up @@ -367,9 +367,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "1",
// "File": "main.gno",
// "Line": "15",
// "Nonce": "0",
// "PkgPath": "gno.land/r/test"
// }
// },
Expand Down
2 changes: 1 addition & 1 deletion gno.land/pkg/sdk/vm/gas_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func TestAddPkgDeliverTx(t *testing.T) {
gasDeliver := gctx.GasMeter().GasConsumed()

assert.True(t, res.IsOK())
assert.Equal(t, int64(87809), gasDeliver)
assert.Equal(t, int64(87929), gasDeliver)
}

// Enough gas for a failed transaction.
Expand Down
4 changes: 2 additions & 2 deletions gnovm/cmd/gno/testdata/gno_test/realm_correct.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "0",
// "File": "",
// "Line": "0",
// "Nonce": "0",
// "PkgPath": "gno.land/r/x"
// }
// },
Expand Down Expand Up @@ -62,9 +62,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "1",
// "File": "main.gno",
// "Line": "6",
// "Nonce": "0",
// "PkgPath": "gno.land/r/x"
// }
// },
Expand Down
4 changes: 2 additions & 2 deletions gnovm/cmd/gno/testdata/gno_test/realm_sync.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "0",
// "File": "",
// "Line": "0",
// "Nonce": "0",
// "PkgPath": "gno.land/r/x"
// }
// },
Expand Down Expand Up @@ -77,9 +77,9 @@ func main() {
// "@type": "/gno.RefNode",
// "BlockNode": null,
// "Location": {
// "Column": "1",
// "File": "main.gno",
// "Line": "6",
// "Nonce": "0",
// "PkgPath": "gno.land/r/x"
// }
// },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

! stdout .+
stderr 'panic: unknown import path net \[recovered\]'
stderr ' panic: gno.land/r/\w{8}/contract.gno:3: unknown import path net'
stderr ' panic: gno.land/r/\w{8}/contract.gno:3:8: unknown import path net'

gno test -v --with-native-fallback .

Expand Down
4 changes: 2 additions & 2 deletions gnovm/cmd/gno/testdata/gno_test/unknow_lib.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@

! stdout .+
stderr 'panic: unknown import path foobarbaz \[recovered\]'
stderr ' panic: gno.land/r/\w{8}/contract.gno:3: unknown import path foobarbaz'
stderr ' panic: gno.land/r/\w{8}/contract.gno:3:8: unknown import path foobarbaz'

! gno test -v --with-native-fallback .

! stdout .+
stderr 'panic: unknown import path foobarbaz \[recovered\]'
stderr ' panic: gno.land/r/\w{8}/contract.gno:3: unknown import path foobarbaz'
stderr ' panic: gno.land/r/\w{8}/contract.gno:3:8: unknown import path foobarbaz'

-- contract.gno --
package contract
Expand Down
4 changes: 2 additions & 2 deletions gnovm/gno.proto
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ message Location {
string PkgPath = 1;
string File = 2;
sint64 Line = 3;
sint64 Nonce = 4;
sint64 Column = 4;
}

message Attributes {
Expand Down Expand Up @@ -600,4 +600,4 @@ message tupleType {

message RefType {
string ID = 1;
}
}
28 changes: 21 additions & 7 deletions gnovm/pkg/gnolang/debugger.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,11 @@ loop:
continue loop
}
default:
for _, b := range m.Debugger.breakpoints {
if b == m.Debugger.loc && m.Debugger.loc != m.Debugger.prevLoc {
m.Debugger.state = DebugAtCmd
m.Debugger.prevLoc = m.Debugger.loc
debugList(m, "")
continue loop
}
if atBreak(m) {
m.Debugger.state = DebugAtCmd
m.Debugger.prevLoc = m.Debugger.loc
debugList(m, "")
continue loop
}
}
break loop
Expand All @@ -159,6 +157,20 @@ loop:
}
}

// atBreak returns true if current machine location matches a breakpoint, false otherwise.
func atBreak(m *Machine) bool {
loc := m.Debugger.loc
if loc == m.Debugger.prevLoc {
return false
}
for _, b := range m.Debugger.breakpoints {
if loc.File == b.File && loc.Line == b.Line {
return true
}
}
return false
}

// debugCmd processes a debugger REPL command. It displays a prompt, then
// reads and parses a command from the debugger input stream, then executes
// the corresponding function or returns an error.
Expand Down Expand Up @@ -231,6 +243,7 @@ func debugUpdateLocation(m *Machine) {
expr := m.Exprs[i]
if l := expr.GetLine(); l > 0 {
m.Debugger.loc.Line = l
m.Debugger.loc.Column = expr.GetColumn()
return
}
}
Expand All @@ -239,6 +252,7 @@ func debugUpdateLocation(m *Machine) {
if stmt := m.PeekStmt1(); stmt != nil {
if l := stmt.GetLine(); l > 0 {
m.Debugger.loc.Line = l
m.Debugger.loc.Column = stmt.GetColumn()
return
}
}
Expand Down
2 changes: 1 addition & 1 deletion gnovm/pkg/gnolang/debugger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ func TestDebug(t *testing.T) {
{in: "b 37\nc\np b\n", out: "(3 int)"},
{in: "b 27\nc\np b\n", out: `("!zero" string)`},
{in: "b 22\nc\np t.A[3]\n", out: "Command failed: slice index out of bounds: 3 (len=3)"},
{in: "b 43\nc\nc\np i\nd\n", out: "(1 int)"},
{in: "b 43\nc\nc\nc\np i\ndetach\n", out: "(1 int)"},
})

runDebugTest(t, "../../tests/files/a1.gno", []dtest{
Expand Down
4 changes: 2 additions & 2 deletions gnovm/pkg/gnolang/gnolang.proto
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ message Location {
string pkg_path = 1 [json_name = "PkgPath"];
string file = 2 [json_name = "File"];
sint64 line = 3 [json_name = "Line"];
sint64 nonce = 4 [json_name = "Nonce"];
sint64 column = 4 [json_name = "Column"];
}

message Attributes {
Expand Down Expand Up @@ -602,4 +602,4 @@ message tupleType {

message RefType {
string id = 1 [json_name = "ID"];
}
}
1 change: 1 addition & 0 deletions gnovm/pkg/gnolang/go2gno.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ func ParseFile(filename string, body string) (fn *FileNode, err error) {
func setLoc(fs *token.FileSet, pos token.Pos, n Node) Node {
posn := fs.Position(pos)
n.SetLine(posn.Line)
n.SetColumn(posn.Column)
return n
}

Expand Down
4 changes: 4 additions & 0 deletions gnovm/pkg/gnolang/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -448,11 +448,13 @@ func (m *Machine) injectLocOnPanic() {
// Show last location information.
// First, determine the line number of expression or statement if any.
lastLine := 0
lastColumn := 0
if len(m.Exprs) > 0 {
for i := len(m.Exprs) - 1; i >= 0; i-- {
expr := m.Exprs[i]
if expr.GetLine() > 0 {
lastLine = expr.GetLine()
lastColumn = expr.GetColumn()
break
}
}
Expand All @@ -462,6 +464,7 @@ func (m *Machine) injectLocOnPanic() {
stmt := m.Stmts[i]
if stmt.GetLine() > 0 {
lastLine = stmt.GetLine()
lastColumn = stmt.GetColumn()
break
}
}
Expand All @@ -476,6 +479,7 @@ func (m *Machine) injectLocOnPanic() {
lastLoc = loc
if lastLine > 0 {
lastLoc.Line = lastLine
lastLoc.Column = lastColumn
}
break
}
Expand Down
41 changes: 22 additions & 19 deletions gnovm/pkg/gnolang/nodes.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,31 +118,23 @@ type Location struct {
PkgPath string
File string
Line int
Nonce int
Column int
}

func (loc Location) String() string {
if loc.Nonce == 0 {
return fmt.Sprintf("%s/%s:%d",
loc.PkgPath,
loc.File,
loc.Line,
)
} else {
return fmt.Sprintf("%s/%s:%d#%d",
loc.PkgPath,
loc.File,
loc.Line,
loc.Nonce,
)
}
return fmt.Sprintf("%s/%s:%d:%d",
loc.PkgPath,
loc.File,
loc.Line,
loc.Column,
)
}

func (loc Location) IsZero() bool {
return loc.PkgPath == "" &&
loc.File == "" &&
loc.Line == 0 &&
loc.Nonce == 0
loc.Column == 0
}

// ----------------------------------------
Expand All @@ -153,9 +145,10 @@ func (loc Location) IsZero() bool {
// for preprocessing) are stored in .data.

type Attributes struct {
Line int
Label Name
data map[interface{}]interface{} // not persisted
Line int
Column int
Label Name
data map[interface{}]interface{} // not persisted
}

func (attr *Attributes) GetLine() int {
Expand All @@ -166,6 +159,14 @@ func (attr *Attributes) SetLine(line int) {
attr.Line = line
}

func (attr *Attributes) GetColumn() int {
return attr.Column
}

func (attr *Attributes) SetColumn(column int) {
attr.Column = column
}

func (attr *Attributes) GetLabel() Name {
return attr.Label
}
Expand Down Expand Up @@ -199,6 +200,8 @@ type Node interface {
Copy() Node
GetLine() int
SetLine(int)
GetColumn() int
SetColumn(int)
GetLabel() Name
SetLabel(Name)
HasAttribute(key interface{}) bool
Expand Down
Loading

0 comments on commit 107d4fc

Please sign in to comment.