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

Issue with defers in elses #3643

Closed
dgryski opened this issue Apr 9, 2023 · 2 comments
Closed

Issue with defers in elses #3643

dgryski opened this issue Apr 9, 2023 · 2 comments

Comments

@dgryski
Copy link
Member

dgryski commented Apr 9, 2023

From Slack:

~/go/src/github.com/u-root/u-root/cmds/core/hexdump $ tinygo test -run='Hex/from_file$' -v
=== RUN   TestHexdump
=== RUN   TestHexdump/hexdump_from_file
opening /var/folders/5b/_hr1d1fd3qn4t9vtfx5p61wm0000gp/T/TestHexdump228065111/000/testfile
returning nil
starting defer chain
defer w
starting defer chain
    --- PASS: TestHexdump/hexdump_from_file (0.00s)
--- PASS: TestHexdump (0.00s)
PASS
ok  	github.com/u-root/u-root/cmds/core/hexdump	0.553s
~/go/src/github.com/u-root/u-root/cmds/core/hexdump $ git diff .
diff --git a/cmds/core/hexdump/hexdump.go b/cmds/core/hexdump/hexdump.go
index d7018ea7..cf2c7156 100644
--- a/cmds/core/hexdump/hexdump.go
+++ b/cmds/core/hexdump/hexdump.go
@@ -31,22 +31,27 @@ func hexdump(filenames []string, reader io.Reader, writer io.Writer) error {
                readers = make([]io.Reader, 0, len(filenames))

                for _, filename := range filenames {
+                       println("opening", filename)
                        f, err := os.Open(filename)
                        if err != nil {
                                return err
                        }
-                       defer f.Close()
+                       defer func() { println("defer file"); f.Close() }()
                        readers = append(readers, f)
                }
        }

        r := io.MultiReader(readers...)
        w := hex.Dumper(writer)
-       defer w.Close()
+       defer func() { println("defer w"); w.Close() }()

        if _, err := io.Copy(w, r); err != nil {
+               println("returning err")
                return err
        }
+       _ = w
+       println("returning nil")
+       defer func() { println("starting defer chain") }()
        return nil
 }

We see starting defer chain twice but no defer file.
The second starting defer chain should be the defer file entry; it's like it gets overwritten.

@dgryski
Copy link
Member Author

dgryski commented Apr 9, 2023

Reproducer:

package main

//go:noinline
func deferbug(b bool) {
        if !b {
                defer println("defer else func")
        }
}

func main() {
	deferbug(false)
}

Running:

~/go/src/github.com/dgryski/bug/defer $ go run main.go
defer else func
~/go/src/github.com/dgryski/bug/defer $ tinygo run main.go
error: failed to run compiled binary /var/folders/5b/_hr1d1fd3qn4t9vtfx5p61wm0000gp/T/tinygo1067563306/main: signal: segmentation fault

@dgryski dgryski changed the title Issue with defers in loops Issue with defers in elses Apr 9, 2023
@dgryski
Copy link
Member Author

dgryski commented Apr 9, 2023

Looks like createRunDefers() is called twice, and depending on exact order of the if blocks in the SSA the appropriate entries might not have been created yet. So .. we need to delay createRunDefers handling until the all the basic blocks have been translated and all the potential defers have been seen.

Here's the SSA for the buggy function:

func deferbug(b bool):
0:                                                                entry P:0 S:2
	if b goto 2 else 1
1:                                                              if.then P:1 S:1
	defer println("defer else func":string)
	jump 2
2:                                                              if.done P:2 S:0
	rundefers
	return
3:                                                              recover P:0 S:0
	return

We first translate the then branch which is 2 which includes the call to rundefers, but the defers haven't been filled in yet. They won't be filled in until we process label 1 in the else block.

dgryski added a commit to dgryski/tinygo that referenced this issue Apr 10, 2023
@dgryski dgryski added the next-release Will be part of next release label Apr 12, 2023
@aykevl aykevl closed this as completed in 4326c8f Jun 10, 2023
@deadprogram deadprogram removed the next-release Will be part of next release label Jun 11, 2023
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants