Skip to content

Commit 73e5235

Browse files
committed
stage2: Pop error trace when storing error to var/const
In order to enforce a strict stack discipline for error return traces, we cannot track error return traces that are stored in variables: ```zig const x = errorable(); // errorable()'s error return trace is killed here // v-- error trace starts here instead return x catch error.UnknownError; ``` In order to propagate error return traces, function calls need to be passed directly to an error-handling expression (`if`, `catch`, `try` or `return`): ```zig // When passed directly to `catch`, the return trace is propagated return errorable() catch error.UnknownError; // Using a break also works return blk: { // code here break :blk errorable(); } catch error.UnknownError; ``` Why do we need this restriction? Without it, multiple errors can co-exist with their own error traces. Handling that situation correctly means either: a. Dynamically allocating trace memory and tracking lifetimes, OR b. Allowing the production of one error to interfere with the trace of another (which is the current status quo) This is piece (3/3) of #1923 (comment)
1 parent a5957dc commit 73e5235

File tree

5 files changed

+147
-42
lines changed

5 files changed

+147
-42
lines changed

lib/test_runner.zig

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -61,23 +61,24 @@ pub fn main() void {
6161
if (!have_tty) {
6262
std.debug.print("{d}/{d} {s}... ", .{ i + 1, test_fn_list.len, test_fn.name });
6363
}
64-
const result = if (test_fn.async_frame_size) |size| switch (io_mode) {
65-
.evented => blk: {
66-
if (async_frame_buffer.len < size) {
67-
std.heap.page_allocator.free(async_frame_buffer);
68-
async_frame_buffer = std.heap.page_allocator.alignedAlloc(u8, std.Target.stack_align, size) catch @panic("out of memory");
69-
}
70-
const casted_fn = @ptrCast(fn () callconv(.Async) anyerror!void, test_fn.func);
71-
break :blk await @asyncCall(async_frame_buffer, {}, casted_fn, .{});
72-
},
73-
.blocking => {
74-
skip_count += 1;
75-
test_node.end();
76-
progress.log("SKIP (async test)\n", .{});
77-
continue;
78-
},
79-
} else test_fn.func();
80-
if (result) |_| {
64+
if (result: {
65+
if (test_fn.async_frame_size) |size| switch (io_mode) {
66+
.evented => {
67+
if (async_frame_buffer.len < size) {
68+
std.heap.page_allocator.free(async_frame_buffer);
69+
async_frame_buffer = std.heap.page_allocator.alignedAlloc(u8, std.Target.stack_align, size) catch @panic("out of memory");
70+
}
71+
const casted_fn = @ptrCast(fn () callconv(.Async) anyerror!void, test_fn.func);
72+
break :result await @asyncCall(async_frame_buffer, {}, casted_fn, .{});
73+
},
74+
.blocking => {
75+
skip_count += 1;
76+
test_node.end();
77+
progress.log("SKIP (async test)\n", .{});
78+
continue;
79+
},
80+
} else break :result test_fn.func();
81+
}) |_| {
8182
ok_count += 1;
8283
test_node.end();
8384
if (!have_tty) std.debug.print("OK\n", .{});

src/AstGen.zig

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8465,9 +8465,22 @@ fn callExpr(
84658465
scratch_index += 1;
84668466
}
84678467

8468+
// If our result location is a try/catch/error-union-if/return, the error trace propagates.
8469+
// Otherwise, it should always be popped (handled in Sema).
8470+
const propagate_error_trace = switch(rl) {
8471+
.catch_none, .catch_ref => true, // Propagate to try/catch/error-union-if
8472+
.ptr, .ty => |ref| b: { // Otherwise, propagate if result loc is a return
8473+
const inst = refToIndex(ref) orelse break :b false;
8474+
const zir_tags = astgen.instructions.items(.tag);
8475+
break :b zir_tags[inst] == .ret_ptr or zir_tags[inst] == .ret_type;
8476+
},
8477+
else => false,
8478+
};
8479+
84688480
const payload_index = try addExtra(astgen, Zir.Inst.Call{
84698481
.callee = callee,
84708482
.flags = .{
8483+
.pop_error_return_trace = !propagate_error_trace,
84718484
.packed_modifier = @intCast(Zir.Inst.Call.Flags.PackedModifier, @enumToInt(modifier)),
84728485
.args_len = @intCast(Zir.Inst.Call.Flags.PackedArgsLen, call.ast.params.len),
84738486
},

src/Sema.zig

Lines changed: 62 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5515,6 +5515,7 @@ fn zirCall(
55155515

55165516
const modifier = @intToEnum(std.builtin.CallOptions.Modifier, extra.data.flags.packed_modifier);
55175517
const ensure_result_used = extra.data.flags.ensure_result_used;
5518+
const pop_error_return_trace = extra.data.flags.pop_error_return_trace;
55185519

55195520
var func = try sema.resolveInst(extra.data.callee);
55205521
var resolved_args: []Air.Inst.Ref = undefined;
@@ -5622,7 +5623,7 @@ fn zirCall(
56225623
resolved_args[arg_index] = resolved;
56235624
}
56245625

5625-
return sema.analyzeCall(block, func, func_src, call_src, modifier, ensure_result_used, resolved_args, bound_arg_src);
5626+
return sema.analyzeCall(block, func, func_src, call_src, modifier, ensure_result_used, pop_error_return_trace, resolved_args, bound_arg_src);
56265627
}
56275628

56285629
const GenericCallAdapter = struct {
@@ -5734,6 +5735,7 @@ fn analyzeCall(
57345735
call_src: LazySrcLoc,
57355736
modifier: std.builtin.CallOptions.Modifier,
57365737
ensure_result_used: bool,
5738+
pop_error_return_trace: bool,
57375739
uncasted_args: []const Air.Inst.Ref,
57385740
bound_arg_src: ?LazySrcLoc,
57395741
) CompileError!Air.Inst.Ref {
@@ -6183,19 +6185,55 @@ fn analyzeCall(
61836185
sema.owner_func.?.calls_or_awaits_errorable_fn = true;
61846186
}
61856187

6186-
try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.Call).Struct.fields.len +
6187-
args.len);
6188-
const func_inst = try block.addInst(.{
6189-
.tag = call_tag,
6190-
.data = .{ .pl_op = .{
6191-
.operand = func,
6192-
.payload = sema.addExtraAssumeCapacity(Air.Call{
6193-
.args_len = @intCast(u32, args.len),
6194-
}),
6195-
} },
6196-
});
6197-
sema.appendRefsAssumeCapacity(args);
6198-
break :res func_inst;
6188+
const backend_supports_error_return_tracing = sema.mod.comp.bin_file.options.use_llvm;
6189+
const emit_error_trace_save_restore = sema.mod.comp.bin_file.options.error_return_tracing and
6190+
backend_supports_error_return_tracing and
6191+
pop_error_return_trace and func_ty_info.return_type.isError();
6192+
6193+
if (emit_error_trace_save_restore) {
6194+
// This function call is error-able (and so can generate an error trace), but AstGen determined
6195+
// that its result does not go to an error-handling operator (try/catch/return etc.). We need to
6196+
// save and restore the error trace index here, effectively "popping" the new entries immediately.
6197+
6198+
const unresolved_stack_trace_ty = try sema.getBuiltinType(block, call_src, "StackTrace");
6199+
const stack_trace_ty = try sema.resolveTypeFields(block, call_src, unresolved_stack_trace_ty);
6200+
const ptr_stack_trace_ty = try Type.Tag.single_mut_pointer.create(sema.arena, stack_trace_ty);
6201+
const err_return_trace = try block.addTy(.err_return_trace, ptr_stack_trace_ty);
6202+
const field_ptr = try sema.structFieldPtr(block, call_src, err_return_trace, "index", call_src, stack_trace_ty, true);
6203+
6204+
const saved_index = try sema.analyzeLoad(block, call_src, field_ptr, call_src);
6205+
6206+
try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.Call).Struct.fields.len +
6207+
args.len);
6208+
const func_inst = try block.addInst(.{
6209+
.tag = call_tag,
6210+
.data = .{ .pl_op = .{
6211+
.operand = func,
6212+
.payload = sema.addExtraAssumeCapacity(Air.Call{
6213+
.args_len = @intCast(u32, args.len),
6214+
}),
6215+
} },
6216+
});
6217+
sema.appendRefsAssumeCapacity(args);
6218+
6219+
try sema.storePtr2(block, call_src, field_ptr, call_src, saved_index, call_src, .store);
6220+
6221+
break :res func_inst;
6222+
} else {
6223+
try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.Call).Struct.fields.len +
6224+
args.len);
6225+
const func_inst = try block.addInst(.{
6226+
.tag = call_tag,
6227+
.data = .{ .pl_op = .{
6228+
.operand = func,
6229+
.payload = sema.addExtraAssumeCapacity(Air.Call{
6230+
.args_len = @intCast(u32, args.len),
6231+
}),
6232+
} },
6233+
});
6234+
sema.appendRefsAssumeCapacity(args);
6235+
break :res func_inst;
6236+
}
61996237
};
62006238

62016239
if (ensure_result_used) {
@@ -10400,7 +10438,7 @@ fn maybeErrorUnwrap(sema: *Sema, block: *Block, body: []const Zir.Inst.Index, op
1040010438
const panic_fn = try sema.getBuiltin(block, src, "panicUnwrapError");
1040110439
const err_return_trace = try sema.getErrorReturnTrace(block, src);
1040210440
const args: [2]Air.Inst.Ref = .{ err_return_trace, operand };
10403-
_ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, &args, null);
10441+
_ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, false, &args, null);
1040410442
return true;
1040510443
},
1040610444
.panic => {
@@ -10411,7 +10449,7 @@ fn maybeErrorUnwrap(sema: *Sema, block: *Block, body: []const Zir.Inst.Index, op
1041110449
const panic_fn = try sema.getBuiltin(block, src, "panic");
1041210450
const err_return_trace = try sema.getErrorReturnTrace(block, src);
1041310451
const args: [2]Air.Inst.Ref = .{ msg_inst, err_return_trace };
10414-
_ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, &args, null);
10452+
_ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, false, &args, null);
1041510453
return true;
1041610454
},
1041710455
else => unreachable,
@@ -15549,7 +15587,7 @@ fn retWithErrTracing(
1554915587
const args: [1]Air.Inst.Ref = .{err_return_trace};
1555015588

1555115589
if (!need_check) {
15552-
_ = try sema.analyzeCall(block, return_err_fn, src, src, .never_inline, false, &args, null);
15590+
_ = try sema.analyzeCall(block, return_err_fn, src, src, .never_inline, false, false, &args, null);
1555315591
_ = try block.addUnOp(ret_tag, operand);
1555415592
return always_noreturn;
1555515593
}
@@ -15560,7 +15598,7 @@ fn retWithErrTracing(
1556015598

1556115599
var else_block = block.makeSubBlock();
1556215600
defer else_block.instructions.deinit(gpa);
15563-
_ = try sema.analyzeCall(&else_block, return_err_fn, src, src, .never_inline, false, &args, null);
15601+
_ = try sema.analyzeCall(&else_block, return_err_fn, src, src, .never_inline, false, false, &args, null);
1556415602
_ = try else_block.addUnOp(ret_tag, operand);
1556515603

1556615604
try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.CondBr).Struct.fields.len +
@@ -19674,7 +19712,7 @@ fn zirBuiltinCall(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError
1967419712
}
1967519713
}
1967619714
const ensure_result_used = extra.flags.ensure_result_used;
19677-
return sema.analyzeCall(block, func, func_src, call_src, modifier, ensure_result_used, resolved_args, bound_arg_src);
19715+
return sema.analyzeCall(block, func, func_src, call_src, modifier, ensure_result_used, false, resolved_args, bound_arg_src);
1967819716
}
1967919717

1968019718
fn zirFieldParentPtr(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Air.Inst.Ref {
@@ -21092,7 +21130,7 @@ fn panicWithMsg(
2109221130
Value.@"null",
2109321131
);
2109421132
const args: [2]Air.Inst.Ref = .{ msg_inst, null_stack_trace };
21095-
_ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, &args, null);
21133+
_ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, false, &args, null);
2109621134
return always_noreturn;
2109721135
}
2109821136

@@ -21133,7 +21171,7 @@ fn panicUnwrapError(
2113321171
const err = try fail_block.addTyOp(unwrap_err_tag, Type.anyerror, operand);
2113421172
const err_return_trace = try sema.getErrorReturnTrace(&fail_block, src);
2113521173
const args: [2]Air.Inst.Ref = .{ err_return_trace, err };
21136-
_ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, &args, null);
21174+
_ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, false, &args, null);
2113721175
}
2113821176
}
2113921177
try sema.addSafetyCheckExtra(parent_block, ok, &fail_block);
@@ -21174,7 +21212,7 @@ fn panicIndexOutOfBounds(
2117421212
} else {
2117521213
const panic_fn = try sema.getBuiltin(&fail_block, src, "panicOutOfBounds");
2117621214
const args: [2]Air.Inst.Ref = .{ index, len };
21177-
_ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, &args, null);
21215+
_ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, false, &args, null);
2117821216
}
2117921217
}
2118021218
try sema.addSafetyCheckExtra(parent_block, ok, &fail_block);
@@ -21216,7 +21254,7 @@ fn panicSentinelMismatch(
2121621254
else {
2121721255
const panic_fn = try sema.getBuiltin(parent_block, src, "checkNonScalarSentinel");
2121821256
const args: [2]Air.Inst.Ref = .{ expected_sentinel, actual_sentinel };
21219-
_ = try sema.analyzeCall(parent_block, panic_fn, src, src, .auto, false, &args, null);
21257+
_ = try sema.analyzeCall(parent_block, panic_fn, src, src, .auto, false, false, &args, null);
2122021258
return;
2122121259
};
2122221260
const gpa = sema.gpa;
@@ -21245,7 +21283,7 @@ fn panicSentinelMismatch(
2124521283
} else {
2124621284
const panic_fn = try sema.getBuiltin(&fail_block, src, "panicSentinelMismatch");
2124721285
const args: [2]Air.Inst.Ref = .{ expected_sentinel, actual_sentinel };
21248-
_ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, &args, null);
21286+
_ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, false, &args, null);
2124921287
}
2125021288
}
2125121289
try sema.addSafetyCheckExtra(parent_block, ok, &fail_block);

src/Zir.zig

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2820,10 +2820,11 @@ pub const Inst = struct {
28202820
pub const Flags = packed struct {
28212821
/// std.builtin.CallOptions.Modifier in packed form
28222822
pub const PackedModifier = u3;
2823-
pub const PackedArgsLen = u28;
2823+
pub const PackedArgsLen = u27;
28242824

28252825
packed_modifier: PackedModifier,
28262826
ensure_result_used: bool = false,
2827+
pop_error_return_trace: bool,
28272828
args_len: PackedArgsLen,
28282829

28292830
comptime {

test/stack_traces.zig

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,58 @@ pub fn addCases(cases: *tests.StackTracesContext) void {
208208
},
209209
});
210210

211+
cases.addCase(.{
212+
.name = "stored errors do not contribute to error trace",
213+
.source =
214+
\\fn foo() !void {
215+
\\ return error.TheSkyIsFalling;
216+
\\}
217+
\\
218+
\\pub fn main() !void {
219+
\\ // Once an error is stored in a variable, it is popped from the trace
220+
\\ var x = foo();
221+
\\ x = {};
222+
\\
223+
\\ // As a result, this error trace will still be clean
224+
\\ return error.SomethingUnrelatedWentWrong;
225+
\\}
226+
,
227+
.Debug = .{
228+
.expect =
229+
\\error: SomethingUnrelatedWentWrong
230+
\\source.zig:11:5: [address] in main (test)
231+
\\ return error.SomethingUnrelatedWentWrong;
232+
\\ ^
233+
\\
234+
,
235+
},
236+
.ReleaseSafe = .{
237+
.exclude_os = .{
238+
.windows, // TODO
239+
.linux, // defeated by aggressive inlining
240+
},
241+
.expect =
242+
\\error: SomethingUnrelatedWentWrong
243+
\\source.zig:11:5: [address] in [function]
244+
\\ return error.SomethingUnrelatedWentWrong;
245+
\\ ^
246+
\\
247+
,
248+
},
249+
.ReleaseFast = .{
250+
.expect =
251+
\\error: SomethingUnrelatedWentWrong
252+
\\
253+
,
254+
},
255+
.ReleaseSmall = .{
256+
.expect =
257+
\\error: SomethingUnrelatedWentWrong
258+
\\
259+
,
260+
},
261+
});
262+
211263
cases.addCase(.{
212264
.name = "try return from within catch",
213265
.source =

0 commit comments

Comments
 (0)