Commit eda3eb1561ec9a68e692821d5de71d03e6f50d42 | zig

author	Cody Tapscott <topolarity@tapscott.me> 2022-09-12 23:05:50-07:00
committer	Cody Tapscott <topolarity@tapscott.me> 2022-10-21 10:43:42-07:00
log	eda3eb1561ec9a68e692821d5de71d03e6f50d42
tree	5a3acd315089bfda58cf4366445ae8ca1916e5fd
parent	5316a00a188955d60cc38d56def51b8605181225

stage2: "Pop" error trace for break/return within catch

This implement trace "popping" for correctly handled errors within
`catch { ... }` and `else { ... }` blocks.

When breaking from these blocks with any non-error, we pop the error
trace frames corresponding to the operand. When breaking with an error,
we preserve the frames so that error traces "chain" together as usual.

```zig
fn foo(cond1: bool, cond2: bool) !void {
    bar() catch {
    	if (cond1) {
	    // If baz() result is a non-error, pop the error trace frames from bar()
	    // If baz() result is an error, leave the bar() frames on the error trace
            return baz();
	} else if (cond2) {
	    // If we break/return an error, then leave the error frames from bar() on the error trace
	    return error.Foo;
	}
    };

    // An error returned from here does not include bar()'s error frames in the trace
    return error.Bar;
}
```

Notice that if foo() does not return an error it, it leaves no extra
frames on the error trace.

This is piece (1/3) of https://github.com/ziglang/zig/issues/1923#issuecomment-1218495574

3 files changed, 301 insertions(+), 29 deletions(-)

src
- AstGen.zig
- Sema.zig
test
- stack_traces.zig

src/AstGen.zig+108-25

 @@ -1834,6 +1834,13 @@ fn breakExpr(parent_gz: *GenZir, parent_scope: *Scope, node: Ast.Node.Index) Inn
     const break_label = node_datas[node].lhs;
     const rhs = node_datas[node].rhs;
+    // Breaking out of a `catch { ... }` or `else |err| { ... }` block with a non-error value
+    // means that the corresponding error was correctly handled, and the error trace index
+    // needs to be restored so that any entries from the caught error are effectively "popped"
+    //
+    // Note: We only restore for the outermost block, since that will "pop" any nested blocks.
+    var err_trace_index_to_restore: Zir.Inst.Ref = .none;
     // Look for the label in the scope.
     var scope = parent_scope;
     while (true) {
 @@ -1842,6 +1849,7 @@ fn breakExpr(parent_gz: *GenZir, parent_scope: *Scope, node: Ast.Node.Index) Inn
                 const block_gz = scope.cast(GenZir).?;
                 if (block_gz.cur_defer_node != 0) {
+                    // We are breaking out of a `defer` block.
                     return astgen.failNodeNotes(node, "cannot break out of defer expression", .{}, &.{
                         try astgen.errNoteNode(
                             block_gz.cur_defer_node,
 @@ -1851,6 +1859,11 @@ fn breakExpr(parent_gz: *GenZir, parent_scope: *Scope, node: Ast.Node.Index) Inn
                     });
                 }
+                if (block_gz.saved_err_trace_index != .none) {
+                    // We are breaking out of a `catch { ... }` or `else |err| { ... }`.
+                    err_trace_index_to_restore = block_gz.saved_err_trace_index;
+                }
                 const block_inst = blk: {
                     if (break_label != 0) {
                         if (block_gz.label) |*label| {
 @@ -1862,9 +1875,11 @@ fn breakExpr(parent_gz: *GenZir, parent_scope: *Scope, node: Ast.Node.Index) Inn
                     } else if (block_gz.break_block != 0) {
                         break :blk block_gz.break_block;
                     }
+                    // If not the target, start over with the parent
                     scope = block_gz.parent;
                     continue;
                 };
+                // If we made it here, this block is the target of the break expr
                 const break_tag: Zir.Inst.Tag = if (block_gz.is_inline or block_gz.force_comptime)
                     .break_inline
 @@ -1874,6 +1889,19 @@ fn breakExpr(parent_gz: *GenZir, parent_scope: *Scope, node: Ast.Node.Index) Inn
                 if (rhs == 0) {
                     try genDefers(parent_gz, scope, parent_scope, .normal_only);
+                    // As our last action before the break, "pop" the error trace if needed
+                    if (err_trace_index_to_restore != .none) {
+                        // TODO: error-liveness and is_non_err
+                        _ = try parent_gz.add(.{
+                            .tag = .restore_err_ret_index,
+                            .data = .{ .un_node = .{
+                                .operand = err_trace_index_to_restore,
+                                .src_node = parent_gz.nodeIndexToRelative(node),
+                            } },
+                        });
+                    }
                     _ = try parent_gz.addBreak(break_tag, block_inst, .void_value);
                     return Zir.Inst.Ref.unreachable_value;
                 }
 @@ -1884,6 +1912,19 @@ fn breakExpr(parent_gz: *GenZir, parent_scope: *Scope, node: Ast.Node.Index) Inn
                 try genDefers(parent_gz, scope, parent_scope, .normal_only);
+                // As our last action before the break, "pop" the error trace if needed
+                if (err_trace_index_to_restore != .none) {
+                    // TODO: error-liveness and is_non_err
+                    _ = try parent_gz.add(.{
+                        .tag = .restore_err_ret_index,
+                        .data = .{ .un_node = .{
+                            .operand = err_trace_index_to_restore,
+                            .src_node = parent_gz.nodeIndexToRelative(node),
+                        } },
+                    });
+                }
                 switch (block_gz.break_result_loc) {
                     .block_ptr => {
                         const br = try parent_gz.addBreak(break_tag, block_inst, operand);
 @@ -5160,9 +5201,7 @@ fn orelseCatchExpr(
     block_scope.setBreakResultLoc(rl);
     defer block_scope.unstack();
-    if (do_err_trace) {
+    const saved_err_trace_index = if (do_err_trace) try parent_gz.addNode(.save_err_ret_index, node) else .none;
-        block_scope.saved_err_trace_index = try parent_gz.addNode(.save_err_ret_index, node);
-    }
     const operand_rl: ResultLoc = switch (block_scope.break_result_loc) {
         .ref => .ref,
 @@ -5195,6 +5234,12 @@ fn orelseCatchExpr(
     var else_scope = block_scope.makeSubBlock(scope);
     defer else_scope.unstack();
+    // Any break (of a non-error value) that navigates out of this scope means
+    // that the error was handled successfully, so this index will be restored.
+    else_scope.saved_err_trace_index = saved_err_trace_index;
+    if (else_scope.outermost_err_trace_index == .none)
+        else_scope.outermost_err_trace_index = saved_err_trace_index;
     var err_val_scope: Scope.LocalVal = undefined;
     const else_sub_scope = blk: {
         const payload = payload_token orelse break :blk &else_scope.base;
 @@ -5220,6 +5265,17 @@ fn orelseCatchExpr(
     const else_result = try expr(&else_scope, else_sub_scope, block_scope.break_result_loc, rhs);
     if (!else_scope.endsWithNoReturn()) {
         block_scope.break_count += 1;
+        // TODO: Add is_non_err and break check
+        if (do_err_trace) {
+            _ = try else_scope.add(.{
+                .tag = .restore_err_ret_index,
+                .data = .{ .un_node = .{
+                    .operand = saved_err_trace_index,
+                    .src_node = parent_gz.nodeIndexToRelative(node),
+                } },
+            });
+        }
     }
     try checkUsed(parent_gz, &else_scope.base, else_sub_scope);
 @@ -5243,15 +5299,6 @@ fn orelseCatchExpr(
         block,
         break_tag,
     );
-    if (do_err_trace) {
-        _ = try parent_gz.add(.{
-            .tag = .restore_err_ret_index,
-            .data = .{ .un_node = .{
-                .operand = parent_gz.saved_err_trace_index,
-                .src_node = parent_gz.nodeIndexToRelative(node),
-            } },
-        });
-    }
     return result;
 }
 @@ -5454,9 +5501,7 @@ fn ifExpr(
     block_scope.setBreakResultLoc(rl);
     defer block_scope.unstack();
-    if (do_err_trace) {
+    const saved_err_trace_index = if (do_err_trace) try parent_gz.addNode(.save_err_ret_index, node) else .none;
-        block_scope.saved_err_trace_index = try parent_gz.addNode(.save_err_ret_index, node);
-    }
     const payload_is_ref = if (if_full.payload_token) |payload_token|
         token_tags[payload_token] == .asterisk
 @@ -5574,6 +5619,12 @@ fn ifExpr(
     var else_scope = parent_gz.makeSubBlock(scope);
     defer else_scope.unstack();
+    // Any break (of a non-error value) that navigates out of this scope means
+    // that the error was handled successfully, so this index will be restored.
+    else_scope.saved_err_trace_index = saved_err_trace_index;
+    if (else_scope.outermost_err_trace_index == .none)
+        else_scope.outermost_err_trace_index = saved_err_trace_index;
     const else_node = if_full.ast.else_expr;
     const else_info: struct {
         src: Ast.Node.Index,
 @@ -5625,6 +5676,18 @@ fn ifExpr(
         },
     };
+    if (do_err_trace and !else_scope.endsWithNoReturn()) {
+        // TODO: is_non_err and other checks
+        _ = try else_scope.add(.{
+            .tag = .restore_err_ret_index,
+            .data = .{ .un_node = .{
+                .operand = saved_err_trace_index,
+                .src_node = parent_gz.nodeIndexToRelative(node),
+            } },
+        });
+    }
     const break_tag: Zir.Inst.Tag = if (parent_gz.force_comptime) .break_inline else .@"break";
     const result = try finishThenElseBlock(
         parent_gz,
 @@ -5641,15 +5704,6 @@ fn ifExpr(
         block,
         break_tag,
     );
-    if (do_err_trace) {
-        _ = try parent_gz.add(.{
-            .tag = .restore_err_ret_index,
-            .data = .{ .un_node = .{
-                .operand = parent_gz.saved_err_trace_index,
-                .src_node = parent_gz.nodeIndexToRelative(node),
-            } },
-        });
-    }
     return result;
 }
 @@ -6780,11 +6834,24 @@ fn ret(gz: *GenZir, scope: *Scope, node: Ast.Node.Index) InnerError!Zir.Inst.Ref
     const operand = try reachableExpr(gz, scope, rl, operand_node, node);
     gz.anon_name_strategy = prev_anon_name_strategy;
+    // TODO: This should be almost identical for every break/ret
     switch (nodeMayEvalToError(tree, operand_node)) {
         .never => {
             // Returning a value that cannot be an error; skip error defers.
             try genDefers(gz, defer_outer, scope, .normal_only);
             try emitDbgStmt(gz, ret_line, ret_column);
+            // As our last action before the return, "pop" the error trace if needed
+            if (gz.outermost_err_trace_index != .none) {
+                _ = try gz.add(.{
+                    .tag = .restore_err_ret_index,
+                    .data = .{ .un_node = .{
+                        .operand = gz.outermost_err_trace_index,
+                        .src_node = gz.nodeIndexToRelative(node),
+                    } },
+                });
+            }
             try gz.addRet(rl, operand, node);
             return Zir.Inst.Ref.unreachable_value;
         },
 @@ -6826,6 +6893,17 @@ fn ret(gz: *GenZir, scope: *Scope, node: Ast.Node.Index) InnerError!Zir.Inst.Ref
             };
             try genDefers(&else_scope, defer_outer, scope, which_ones);
             try emitDbgStmt(&else_scope, ret_line, ret_column);
+            // As our last action before the return, "pop" the error trace if needed
+            if (else_scope.outermost_err_trace_index != .none) {
+                _ = try else_scope.add(.{
+                    .tag = .restore_err_ret_index,
+                    .data = .{ .un_node = .{
+                        .operand = else_scope.outermost_err_trace_index,
+                        .src_node = else_scope.nodeIndexToRelative(node),
+                    } },
+                });
+            }
             try else_scope.addRet(rl, operand, node);
             try setCondBrPayload(condbr, is_non_err, &then_scope, 0, &else_scope, 0);
 @@ -10334,7 +10412,12 @@ const GenZir = struct {
     /// Keys are the raw instruction index, values are the closure_capture instruction.
     captures: std.AutoHashMapUnmanaged(Zir.Inst.Index, Zir.Inst.Index) = .{},
+    /// If this GenZir corresponds to a `catch { ... }` or `else |err| { ... }` block,
+    /// this err_trace_index can be restored to "pop" the trace entries for the block.
     saved_err_trace_index: Zir.Inst.Ref = .none,
+    /// When returning from a function with a non-error, we must pop all trace entries
+    /// from any containing `catch { ... }` or `else |err| { ... }` blocks.
+    outermost_err_trace_index: Zir.Inst.Ref = .none,
     const unstacked_top = std.math.maxInt(usize);
     /// Call unstack before adding any new instructions to containing GenZir.
 @@ -10380,7 +10463,7 @@ const GenZir = struct {
             .any_defer_node = gz.any_defer_node,
             .instructions = gz.instructions,
             .instructions_top = gz.instructions.items.len,
-            .saved_err_trace_index = gz.saved_err_trace_index,
+            .outermost_err_trace_index = gz.outermost_err_trace_index,
         };
     }

src/Sema.zig+8-4

 @@ -16190,9 +16190,14 @@ fn zirSaveErrRetIndex(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileE
     // This is only relevant at runtime.
     if (block.is_comptime) return Air.Inst.Ref.zero_usize;
+    // In the corner case that `catch { ... }` or `else |err| { ... }` is used in a function
+    // that does *not* make any errorable calls, we still need an error trace to interact with
+    // the AIR instructions we've already emitted.
+    if (sema.owner_func != null)
+        sema.owner_func.?.calls_or_awaits_errorable_fn = true;
     const backend_supports_error_return_tracing = sema.mod.comp.bin_file.options.use_llvm;
-    const ok = sema.owner_func.?.calls_or_awaits_errorable_fn and
+    const ok = sema.mod.comp.bin_file.options.error_return_tracing and
-        sema.mod.comp.bin_file.options.error_return_tracing and
         backend_supports_error_return_tracing;
     if (!ok) return Air.Inst.Ref.zero_usize;
 @@ -16211,8 +16216,7 @@ fn zirRestoreErrRetIndex(sema: *Sema, block: *Block, inst: Zir.Inst.Index) Compi
     if (block.is_comptime) return;
     const backend_supports_error_return_tracing = sema.mod.comp.bin_file.options.use_llvm;
-    const ok = sema.owner_func.?.calls_or_awaits_errorable_fn and
+    const ok = sema.mod.comp.bin_file.options.error_return_tracing and
-        sema.mod.comp.bin_file.options.error_return_tracing and
         backend_supports_error_return_tracing;
     if (!ok) return;

test/stack_traces.zig+185

...	@@ -98,6 +98,191 @@ pub fn addCases(cases: *tests.StackTracesContext) void {	...	@@ -98,6 +98,191 @@ pub fn addCases(cases: *tests.StackTracesContext) void {
98	},	98	},
99	});	99	});
100		100
		101	cases.addCase(.{
		102	.name = "try return + handled catch/if-else",
		103	.source =
		104	\\fn foo() !void {
		105	\\ return error.TheSkyIsFalling;
		106	\\}
		107	\\
		108	\\pub fn main() !void {
		109	\\ foo() catch {}; // should not affect error trace
		110	\\ if (foo()) \|_\| {} else \|_\| {
		111	\\ // should also not affect error trace
		112	\\ }
		113	\\ try foo();
		114	\\}
		115	,
		116	.Debug = .{
		117	.expect =
		118	\\error: TheSkyIsFalling
		119	\\source.zig:2:5: [address] in foo (test)
		120	\\ return error.TheSkyIsFalling;
		121	\\ ^
		122	\\source.zig:10:5: [address] in main (test)
		123	\\ try foo();
		124	\\ ^
		125	\\
		126	,
		127	},
		128	.ReleaseSafe = .{
		129	.exclude_os = .{
		130	.windows, // TODO
		131	.linux, // defeated by aggressive inlining
		132	},
		133	.expect =
		134	\\error: TheSkyIsFalling
		135	\\source.zig:2:5: [address] in [function]
		136	\\ return error.TheSkyIsFalling;
		137	\\ ^
		138	\\source.zig:10:5: [address] in [function]
		139	\\ try foo();
		140	\\ ^
		141	\\
		142	,
		143	},
		144	.ReleaseFast = .{
		145	.expect =
		146	\\error: TheSkyIsFalling
		147	\\
		148	,
		149	},
		150	.ReleaseSmall = .{
		151	.expect =
		152	\\error: TheSkyIsFalling
		153	\\
		154	,
		155	},
		156	});
		157
		158	cases.addCase(.{
		159	.name = "try return from within catch",
		160	.source =
		161	\\fn foo() !void {
		162	\\ return error.TheSkyIsFalling;
		163	\\}
		164	\\
		165	\\fn bar() !void {
		166	\\ return error.AndMyCarIsOutOfGas;
		167	\\}
		168	\\
		169	\\pub fn main() !void {
		170	\\ foo() catch { // error trace should include foo()
		171	\\ try bar();
		172	\\ };
		173	\\}
		174	,
		175	.Debug = .{
		176	.expect =
		177	\\error: AndMyCarIsOutOfGas
		178	\\source.zig:2:5: [address] in foo (test)
		179	\\ return error.TheSkyIsFalling;
		180	\\ ^
		181	\\source.zig:6:5: [address] in bar (test)
		182	\\ return error.AndMyCarIsOutOfGas;
		183	\\ ^
		184	\\source.zig:11:9: [address] in main (test)
		185	\\ try bar();
		186	\\ ^
		187	\\
		188	,
		189	},
		190	.ReleaseSafe = .{
		191	.exclude_os = .{
		192	.windows, // TODO
		193	},
		194	.expect =
		195	\\error: AndMyCarIsOutOfGas
		196	\\source.zig:2:5: [address] in [function]
		197	\\ return error.TheSkyIsFalling;
		198	\\ ^
		199	\\source.zig:6:5: [address] in [function]
		200	\\ return error.AndMyCarIsOutOfGas;
		201	\\ ^
		202	\\source.zig:11:9: [address] in [function]
		203	\\ try bar();
		204	\\ ^
		205	\\
		206	,
		207	},
		208	.ReleaseFast = .{
		209	.expect =
		210	\\error: AndMyCarIsOutOfGas
		211	\\
		212	,
		213	},
		214	.ReleaseSmall = .{
		215	.expect =
		216	\\error: AndMyCarIsOutOfGas
		217	\\
		218	,
		219	},
		220	});
		221
		222	cases.addCase(.{
		223	.name = "try return from within if-else",
		224	.source =
		225	\\fn foo() !void {
		226	\\ return error.TheSkyIsFalling;
		227	\\}
		228	\\
		229	\\fn bar() !void {
		230	\\ return error.AndMyCarIsOutOfGas;
		231	\\}
		232	\\
		233	\\pub fn main() !void {
		234	\\ if (foo()) \|_\| {} else \|_\| { // error trace should include foo()
		235	\\ try bar();
		236	\\ }
		237	\\}
		238	,
		239	.Debug = .{
		240	.expect =
		241	\\error: AndMyCarIsOutOfGas
		242	\\source.zig:2:5: [address] in foo (test)
		243	\\ return error.TheSkyIsFalling;
		244	\\ ^
		245	\\source.zig:6:5: [address] in bar (test)
		246	\\ return error.AndMyCarIsOutOfGas;
		247	\\ ^
		248	\\source.zig:11:9: [address] in main (test)
		249	\\ try bar();
		250	\\ ^
		251	\\
		252	,
		253	},
		254	.ReleaseSafe = .{
		255	.exclude_os = .{
		256	.windows, // TODO
		257	},
		258	.expect =
		259	\\error: AndMyCarIsOutOfGas
		260	\\source.zig:2:5: [address] in [function]
		261	\\ return error.TheSkyIsFalling;
		262	\\ ^
		263	\\source.zig:6:5: [address] in [function]
		264	\\ return error.AndMyCarIsOutOfGas;
		265	\\ ^
		266	\\source.zig:11:9: [address] in [function]
		267	\\ try bar();
		268	\\ ^
		269	\\
		270	,
		271	},
		272	.ReleaseFast = .{
		273	.expect =
		274	\\error: AndMyCarIsOutOfGas
		275	\\
		276	,
		277	},
		278	.ReleaseSmall = .{
		279	.expect =
		280	\\error: AndMyCarIsOutOfGas
		281	\\
		282	,
		283	},
		284	});
		285
101	cases.addCase(.{	286	cases.addCase(.{
102	.name = "try try return return",	287	.name = "try try return return",
103	.source =	288	.source =