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.

Sign up for GitHub

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

Jump to bottom

interp: recover in Eval and EvalPath #1560

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

interp: recover in Eval and EvalPath #1560

wants to merge 10 commits into from

Conversation

Bai-Yingjie
Copy link
Contributor

Now Eval() and EvalPath() recover from internal panic and return it as error with call stacks.

This fixes the case where user code calls os.Exit(1), which is converted to panic() and breaks down the program.

@mvertes
Copy link
Member

mvertes commented Jun 16, 2023

Can you provide a test program which fails before and works after your change request?

I'm not able to reproduce the issue you mention. We already have the panic wrapped in an error in the current code as far as I see.

@Bai-Yingjie
Copy link
Contributor Author

Thanks for your review, and sorry for the late response...

To preproduce the panic, put below code to a file in a directory, e.g. ./test/hello.go:

package main

import (
        "fmt"
        "os"
)

func main() {
        fmt.Println("Hello, playground")
        os.Exit(1)
}

Then run yaegi with the directory can reproduce the panic:

$ ./yaegi ./test
Hello, playground
test/hello.go:9:2: panic
panic: os.Exit(1) [recovered]
        panic: os.Exit(1)

goroutine 1 [running]:
github.com/traefik/yaegi/interp.runCfg.func1()
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:205 +0x12e
panic({0xd47640, 0xc00007e970})
        /usr/local/go/src/runtime/panic.go:838 +0x207
github.com/traefik/yaegi/stdlib.osExit(0x0?)
        /repo/yingjieb/3rdparty/yaegi/stdlib/restricted.go:14 +0x59
reflect.Value.call({0xd45a80?, 0xecb318?, 0x410225?}, {0xe649b0, 0x4}, {0xc00000c678, 0x1, 0x4cbc05?})
        /usr/local/go/src/reflect/value.go:556 +0x845
reflect.Value.Call({0xd45a80?, 0xecb318?, 0x453452?}, {0xc00000c678, 0x1, 0x1})
        /usr/local/go/src/reflect/value.go:339 +0xbf
github.com/traefik/yaegi/interp.callBin.func2({0xd45a80?, 0xecb318?, 0xc000245620?}, {0xc00000c678?, 0xc00007e910?, 0xc000245638?})
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:1453 +0x28
github.com/traefik/yaegi/interp.callBin.func11(0xc00037c160)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:1641 +0x15f
github.com/traefik/yaegi/interp.runCfg(0xc0003afa40, 0xc00037c160, 0x0?, 0xc0002457f8?)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:213 +0x29d
github.com/traefik/yaegi/interp.(*Interpreter).run(0xc000032480, 0xc0003aef00, 0xc00037c000?)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:119 +0x374
github.com/traefik/yaegi/interp.(*Interpreter).importSrc(0xc000032480, {0xe650cc, 0x4}, {0x7ffe66ad0c0e, 0x6}, 0x1)
        /repo/yingjieb/3rdparty/yaegi/interp/src.go:170 +0xb55
github.com/traefik/yaegi/interp.(*Interpreter).EvalPath(0xc000032480, {0x7ffe66ad0c0e, 0x6})
        /repo/yingjieb/3rdparty/yaegi/interp/interp.go:509 +0xdb
main.run({0xc0001a8010?, 0x1, 0x1})
        /repo/yingjieb/3rdparty/yaegi/cmd/yaegi/run.go:118 +0xc0b
main.main()
        /repo/yingjieb/3rdparty/yaegi/cmd/yaegi/yaegi.go:144 +0x2dc

When executing a directory, EvalPath() is used:

EvalPath()
    interp.importSrc()

interp.importSrc() compiles all the files in the directory and calls interp.run() to run the main() function. There is a defer recover deep in the function runCfg(), but it re-triggers the panic:
image

The top level EvalPath() does not defer & recover the panic, which caused the whole process to die immediately.

On the other hand, when executing a file, although the program also exits, but it does not die because of panic, but because the panic was recovered in interp.Execute() so the program can exit normally.

Eval()
    interp.compileSrc()
    interp.Execute()

It prints the call stack similar with the above one, but it is not a direct result of panic, it is just the returned error of Eval()

$ ./yaegi ./test/hello.go
Hello, playground
./test/hello.go:9:2: panic
run: os.Exit(1)
goroutine 1 [running]:
runtime/debug.Stack()
        /usr/local/go/src/runtime/debug/stack.go:24 +0x65
github.com/traefik/yaegi/interp.(*Interpreter).Execute.func1()
        /repo/yingjieb/3rdparty/yaegi/interp/program.go:146 +0x94
panic({0xd47640, 0xc00011a920})
        /usr/local/go/src/runtime/panic.go:838 +0x207
github.com/traefik/yaegi/interp.runCfg.func1()
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:205 +0x12e
panic({0xd47640, 0xc00011a920})
        /usr/local/go/src/runtime/panic.go:838 +0x207
github.com/traefik/yaegi/stdlib.osExit(0x0?)
        /repo/yingjieb/3rdparty/yaegi/stdlib/restricted.go:14 +0x59
reflect.Value.call({0xd45a80?, 0xecb318?, 0x410225?}, {0xe649b0, 0x4}, {0xc000126630, 0x1, 0x4cbc05?})
        /usr/local/go/src/reflect/value.go:556 +0x845
reflect.Value.Call({0xd45a80?, 0xecb318?, 0x453452?}, {0xc000126630, 0x1, 0x1})
        /usr/local/go/src/reflect/value.go:339 +0xbf
github.com/traefik/yaegi/interp.callBin.func2({0xd45a80?, 0xecb318?, 0xc0001cf8c8?}, {0xc000126630?, 0xc00011a8c0?, 0xc0001cf8e0?})
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:1453 +0x28
github.com/traefik/yaegi/interp.callBin.func11(0xc00037c160)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:1641 +0x15f
github.com/traefik/yaegi/interp.runCfg(0xc0003afa40, 0xc00037c160, 0x0?, 0x0?)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:213 +0x29d
github.com/traefik/yaegi/interp.(*Interpreter).run(0xc00037a240, 0xc0003aef00, 0xc00037c000?)
        /repo/yingjieb/3rdparty/yaegi/interp/run.go:119 +0x374
github.com/traefik/yaegi/interp.(*Interpreter).Execute(0xc00037a240, 0xc000457650)
        /repo/yingjieb/3rdparty/yaegi/interp/program.go:172 +0x24b
github.com/traefik/yaegi/interp.(*Interpreter).eval(0xc00037a240, {0xc0002dcab0?, 0x85?}, {0x7fff02a91c05?, 0xc0002dc990?}, 0x85?)
        /repo/yingjieb/3rdparty/yaegi/interp/interp.go:568 +0x5c
github.com/traefik/yaegi/interp.(*Interpreter).EvalPath(0xc00037a240, {0x7fff02a91c05, 0xf})
        /repo/yingjieb/3rdparty/yaegi/interp/interp.go:517 +0xab
main.runFile(0x7fff02a91c05?, {0x7fff02a91c05, 0xf}, 0x0)
        /repo/yingjieb/3rdparty/yaegi/cmd/yaegi/run.go:153 +0xee
main.run({0xc000030050?, 0x1, 0x1})
        /repo/yingjieb/3rdparty/yaegi/cmd/yaegi/run.go:116 +0xbec
main.main()
        /repo/yingjieb/3rdparty/yaegi/cmd/yaegi/yaegi.go:144 +0x2dc

In a program like gshell, we use yaegi to execute go source codes inside a goroutine, it is better that the Eval() and EvalPath() never panics directly, but always recovers the panic at top level and returns the error, so that the whole program will not break because of panic.

@Bai-Yingjie
Copy link
Contributor Author

@mvertes Is there any other steps should I do to deliver the patch? I see all checks have passed but review approval is required.

@Bai-Yingjie
interp: recover in Eval and EvalPath
48f8875 
Now Eval() and EvalPath() recover from internal panic and
return it as error with call stacks.

This fixes the case where user code calls os.Exit(1), which is
converted to panic() and breaks down the program.
mvertes and others added 4 commits July 13, 2024 12:51
@mvertes @Bai-Yingjie
fix: do not panic when assigning to _ (blank) var.
3f6b949 
Fix interp.isEmptyInterface to tolerate a nil type.

Fixes traefik#1619
@mvertes @Bai-Yingjie
fix: avoid memory leak in closure
d9d2d61 
Simplify frame management. Remove node dependency on frame pointer.

Fixes traefik#1618
@mvertes @Bai-Yingjie
fix: don't panic in map range if assigned value is _
8e18c46 
Ensure that the code generated for `for s, _ = range ...` is the same as `for s = range ...`.

Fixes traefik#1622.
@theclapp @Bai-Yingjie
interp: allow assignment to exported variables
9ed60ff 
If you `(*interp.Interpreter).Use` a variable, you should be able to assign to it.

Fixes traefik#1623
mvertes and others added 5 commits October 9, 2024 09:02
@mvertes @Bai-Yingjie
interp: fix handling of redeclared variables in short declaration.
86d4fa1 
Fixes traefik#1640.
@chikaku @Bai-Yingjie
interp: fix mismatch assign statement panic
bf5850f 
Follow by the [Spec](https://go.dev/ref/spec#Assignment_statements):

The number of operands on the left hand side must match the number of values. For instance, if f is a function returning two values `x, y = f()` assigns the first value to x and the second to y.

In the second form, the number of operands on the left must equal the number of expressions on the right, each of which must be single-valued, and the nth expression on the right is assigned to the nth operand on the left.

Fixes traefik#1606
@rcoreilly @Bai-Yingjie
fix for issue traefik#1634 -- for passing a closure to exported Go fu…
3efbf2b 
…nction

This fixes issue traefik#1634 

includes special case for defer function.

I could remove or significantly reduce the comment description, and just have that here for future reference:

// per traefik#1634, if v is already a func, then don't re-wrap!  critically, the original wrapping
// clones the frame, whereas the one here (below) does _not_ clone the frame, so it doesn't
// generate the proper closure capture effects!
// this path is the same as genValueAsFunctionWrapper which is the path taken above if
// the value has an associated node, which happens when you do f := func() ..
@rcoreilly @Bai-Yingjie
automatic loop variables in for loops ("loop var"), consistent with g…
40a459c 
…o 1.22 behavior

This builds on traefik#1644 and adds automatic per-loop variables that are consistent with go 1.22 behavior.  See traefik#1643 for discussion.

This is still a draft because the for7 version ends up capturing the per-loop var values when they are +1 relative to what they should be.  Maybe somehow the incrementing and conditional code is somehow capturing the within loop variables and incrementing them?  not sure how that would work.  anyway, need to investigate further before this is ready to go, but pushing it here in case there are other issues or someone might figure out this bug before I do..
@mvertes @Bai-Yingjie
interp: fix a missing implicit type conversion for binary expression
ee47504 
When parsing binary operator expressions, make sure that implicit type conversions for untyped expressions are performed. It involves walking the sub-expression subtree at post-processing of binary expressions.

Fixes traefik#1653.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Bai-Yingjie @mvertes @theclapp @chikaku @rcoreilly