ssa,test: lower builtin panic in defer paths#1839
Conversation
There was a problem hiding this comment.
Review Summary
Clean, well-scoped change that correctly routes the panic builtin through Builder.Panic when it appears as a deferred builtin call (e.g., defer panic("msg")). Without this fix, such calls would fall through to panic("todo: " + fn), crashing the compiler.
The test uses both CHECK directives for IR verification and expect.txt for runtime output verification, which is good.
Two minor nits below.
| panic("invalid argument for unsafe.Offsetof: must be a selector expression") | ||
| case "panic": |
There was a problem hiding this comment.
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.
| case "panic": | ||
| b.Panic(args[0]) | ||
| return | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Code Review
This pull request implements support for the panic built-in function within the SSA builder and includes a new test case to verify its behavior. The feedback suggests adding an argument count validation for the panic built-in to ensure robustness and consistency with other built-in function implementations in the SSA builder.
| } | ||
| panic("invalid argument for unsafe.Offsetof: must be a selector expression") | ||
| case "panic": | ||
| b.Panic(args[0]) |
There was a problem hiding this comment.
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])Handle builtin panic calls during SSA lowering and add a defer/recover regression case for panic raised from a deferred call. Signed-off-by: ZhouGuangyuan <zhouguangyuan.xian@gmail.com>
f3d51bd to
7b18956
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1839 +/- ##
=======================================
Coverage 88.50% 88.50%
=======================================
Files 50 50
Lines 14426 14429 +3
=======================================
+ Hits 12768 12771 +3
Misses 1438 1438
Partials 220 220 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
paniccalls throughBuilder.Panicduring SSA loweringcl/_testdata/deferpaniccoverage for panic raised from a deferred callrecover()path prints the expected outputTesting