Skip to content

ssa,test: lower builtin panic in defer paths #1839

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

Merged
xushiwei merged 1 commit into from
May 2, 2026
Merged

ssa,test: lower builtin panic in defer paths #1839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions cl/_testlibgo/deferpanic/expect.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
run main
panic in defer
13 changes: 13 additions & 0 deletions cl/_testlibgo/deferpanic/in.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// LITTEST
package main

// CHECK: define void @"{{.*}}/deferpanic.main"(){{.*}} {
func main() {
defer func() {
e := recover()
println(e.(string))
}()
// CHECK: call void @"{{.*}}/runtime/internal/runtime.Panic"
defer panic("panic in defer")
println("run main")
}
4 changes: 4 additions & 0 deletions ssa/expr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1473,7 +1473,11 @@ func (b Builder) BuiltinCall(fn string, args ...Expr) (ret Expr) {
}
}
panic("invalid argument for unsafe.Offsetof: must be a selector expression")
case "panic":
Comment on lines 1475 to +1476
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot May 1, 2026

Choose a reason for hiding this comment

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

Nit: This accesses args[0] without a length guard. Other void-returning builtins in this switch (delete, close, clear) use guards like if len(args) == 1. Since the Go type checker guarantees panic always has exactly one argument, this is safe — and other fixed-arity builtins (complex, real, imag) also skip the guard. Still, adding if len(args) == 1 { would be defensive and self-documenting about the expected arity.

Not a bug, just a consistency observation.

b.Panic(args[0])
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 1, 2026

Choose a reason for hiding this comment

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

medium

For robustness, it's good practice to validate the number of arguments for built-in functions. The panic function expects exactly one argument. While the frontend should enforce this, adding a check here would make the SSA builder more resilient to potential issues in earlier compilation stages. Other built-ins like len, cap, and close in this file already have similar checks.

		if len(args) != 1 {
			panic("ssa: builtin panic expects 1 argument")
		}
		b.Panic(args[0])

return
}
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot May 1, 2026

Choose a reason for hiding this comment

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

Nit: This blank line was not present before and is inconsistent with the original formatting (the closing } of the switch was immediately followed by panic("todo: " + fn) with no blank line).


panic("todo: " + fn)
}

Expand Down
Loading