From 3007fdde45868142654d0bfa59bc0e17e5f24a1c Mon Sep 17 00:00:00 2001 From: Cody Tapscott Date: Tue, 13 Sep 2022 15:31:07 -0700 Subject: [PATCH] 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 https://github.com/ziglang/zig/issues/1923#issuecomment-1218495574 --- lib/test_runner.zig | 35 +++++++++--------- src/AstGen.zig | 13 +++++++ src/Sema.zig | 86 +++++++++++++++++++++++++++++++------------ src/Zir.zig | 3 +- test/stack_traces.zig | 52 ++++++++++++++++++++++++++ 5 files changed, 147 insertions(+), 42 deletions(-) diff --git a/lib/test_runner.zig b/lib/test_runner.zig index aafaf1b07389f7c3c898b86b0a003124af54722a..8cde13f9a64de658f8b888990fdfb65a5a36f3d7 100644 --- a/lib/test_runner.zig +++ b/lib/test_runner.zig @@ -44,23 +44,24 @@ pub fn main() void { if (!have_tty) { std.debug.print("{d}/{d} {s}... ", .{ i + 1, test_fn_list.len, test_fn.name }); } - const result = if (test_fn.async_frame_size) |size| switch (io_mode) { - .evented => blk: { - if (async_frame_buffer.len < size) { - std.heap.page_allocator.free(async_frame_buffer); - async_frame_buffer = std.heap.page_allocator.alignedAlloc(u8, std.Target.stack_align, size) catch @panic("out of memory"); - } - const casted_fn = @ptrCast(fn () callconv(.Async) anyerror!void, test_fn.func); - break :blk await @asyncCall(async_frame_buffer, {}, casted_fn, .{}); - }, - .blocking => { - skip_count += 1; - test_node.end(); - progress.log("SKIP (async test)\n", .{}); - continue; - }, - } else test_fn.func(); - if (result) |_| { + if (result: { + if (test_fn.async_frame_size) |size| switch (io_mode) { + .evented => { + if (async_frame_buffer.len < size) { + std.heap.page_allocator.free(async_frame_buffer); + async_frame_buffer = std.heap.page_allocator.alignedAlloc(u8, std.Target.stack_align, size) catch @panic("out of memory"); + } + const casted_fn = @ptrCast(fn () callconv(.Async) anyerror!void, test_fn.func); + break :result await @asyncCall(async_frame_buffer, {}, casted_fn, .{}); + }, + .blocking => { + skip_count += 1; + test_node.end(); + progress.log("SKIP (async test)\n", .{}); + continue; + }, + } else break :result test_fn.func(); + }) |_| { ok_count += 1; test_node.end(); if (!have_tty) std.debug.print("OK\n", .{}); diff --git a/src/AstGen.zig b/src/AstGen.zig index b7a035902a3d04a15ae82768cc896752a430cec0..ec67a8981008656d0ab9ef06e4b28bf0e9658aa5 100644 --- a/src/AstGen.zig +++ b/src/AstGen.zig @@ -8545,9 +8545,22 @@ fn callExpr( scratch_index += 1; } + // If our result location is a try/catch/error-union-if/return, the error trace propagates. + // Otherwise, it should always be popped (handled in Sema). + const propagate_error_trace = switch (rl) { + .catch_none, .catch_ref => true, // Propagate to try/catch/error-union-if + .ptr, .ty => |ref| b: { // Otherwise, propagate if result loc is a return + const inst = refToIndex(ref) orelse break :b false; + const zir_tags = astgen.instructions.items(.tag); + break :b zir_tags[inst] == .ret_ptr or zir_tags[inst] == .ret_type; + }, + else => false, + }; + const payload_index = try addExtra(astgen, Zir.Inst.Call{ .callee = callee, .flags = .{ + .pop_error_return_trace = !propagate_error_trace, .packed_modifier = @intCast(Zir.Inst.Call.Flags.PackedModifier, @enumToInt(modifier)), .args_len = @intCast(Zir.Inst.Call.Flags.PackedArgsLen, call.ast.params.len), }, diff --git a/src/Sema.zig b/src/Sema.zig index ce3a10fd386c09f4aa6942ee8279d1a9ec5c9ce6..93f496a43ad8c03e0130785397dd0e6ad5c05e9b 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -5664,6 +5664,7 @@ fn zirCall( const modifier = @intToEnum(std.builtin.CallOptions.Modifier, extra.data.flags.packed_modifier); const ensure_result_used = extra.data.flags.ensure_result_used; + const pop_error_return_trace = extra.data.flags.pop_error_return_trace; var func = try sema.resolveInst(extra.data.callee); var resolved_args: []Air.Inst.Ref = undefined; @@ -5771,7 +5772,7 @@ fn zirCall( resolved_args[arg_index] = resolved; } - return sema.analyzeCall(block, func, func_src, call_src, modifier, ensure_result_used, resolved_args, bound_arg_src); + return sema.analyzeCall(block, func, func_src, call_src, modifier, ensure_result_used, pop_error_return_trace, resolved_args, bound_arg_src); } const GenericCallAdapter = struct { @@ -5883,6 +5884,7 @@ fn analyzeCall( call_src: LazySrcLoc, modifier: std.builtin.CallOptions.Modifier, ensure_result_used: bool, + pop_error_return_trace: bool, uncasted_args: []const Air.Inst.Ref, bound_arg_src: ?LazySrcLoc, ) CompileError!Air.Inst.Ref { @@ -6333,19 +6335,55 @@ fn analyzeCall( sema.owner_func.?.calls_or_awaits_errorable_fn = true; } - try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.Call).Struct.fields.len + - args.len); - const func_inst = try block.addInst(.{ - .tag = call_tag, - .data = .{ .pl_op = .{ - .operand = func, - .payload = sema.addExtraAssumeCapacity(Air.Call{ - .args_len = @intCast(u32, args.len), - }), - } }, - }); - sema.appendRefsAssumeCapacity(args); - break :res func_inst; + const backend_supports_error_return_tracing = sema.mod.comp.bin_file.options.use_llvm; + const emit_error_trace_save_restore = sema.mod.comp.bin_file.options.error_return_tracing and + backend_supports_error_return_tracing and + pop_error_return_trace and func_ty_info.return_type.isError(); + + if (emit_error_trace_save_restore) { + // This function call is error-able (and so can generate an error trace), but AstGen determined + // that its result does not go to an error-handling operator (try/catch/return etc.). We need to + // save and restore the error trace index here, effectively "popping" the new entries immediately. + + const unresolved_stack_trace_ty = try sema.getBuiltinType(block, call_src, "StackTrace"); + const stack_trace_ty = try sema.resolveTypeFields(block, call_src, unresolved_stack_trace_ty); + const ptr_stack_trace_ty = try Type.Tag.single_mut_pointer.create(sema.arena, stack_trace_ty); + const err_return_trace = try block.addTy(.err_return_trace, ptr_stack_trace_ty); + const field_ptr = try sema.structFieldPtr(block, call_src, err_return_trace, "index", call_src, stack_trace_ty, true); + + const saved_index = try sema.analyzeLoad(block, call_src, field_ptr, call_src); + + try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.Call).Struct.fields.len + + args.len); + const func_inst = try block.addInst(.{ + .tag = call_tag, + .data = .{ .pl_op = .{ + .operand = func, + .payload = sema.addExtraAssumeCapacity(Air.Call{ + .args_len = @intCast(u32, args.len), + }), + } }, + }); + sema.appendRefsAssumeCapacity(args); + + try sema.storePtr2(block, call_src, field_ptr, call_src, saved_index, call_src, .store); + + break :res func_inst; + } else { + try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.Call).Struct.fields.len + + args.len); + const func_inst = try block.addInst(.{ + .tag = call_tag, + .data = .{ .pl_op = .{ + .operand = func, + .payload = sema.addExtraAssumeCapacity(Air.Call{ + .args_len = @intCast(u32, args.len), + }), + } }, + }); + sema.appendRefsAssumeCapacity(args); + break :res func_inst; + } }; if (ensure_result_used) { @@ -10927,7 +10965,7 @@ fn maybeErrorUnwrap(sema: *Sema, block: *Block, body: []const Zir.Inst.Index, op const panic_fn = try sema.getBuiltin(block, src, "panicUnwrapError"); const err_return_trace = try sema.getErrorReturnTrace(block, src); const args: [2]Air.Inst.Ref = .{ err_return_trace, operand }; - _ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, &args, null); + _ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, false, &args, null); return true; }, .panic => { @@ -10938,7 +10976,7 @@ fn maybeErrorUnwrap(sema: *Sema, block: *Block, body: []const Zir.Inst.Index, op const panic_fn = try sema.getBuiltin(block, src, "panic"); const err_return_trace = try sema.getErrorReturnTrace(block, src); const args: [3]Air.Inst.Ref = .{ msg_inst, err_return_trace, .null_value }; - _ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, &args, null); + _ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, false, &args, null); return true; }, else => unreachable, @@ -16141,7 +16179,7 @@ fn retWithErrTracing( const args: [1]Air.Inst.Ref = .{err_return_trace}; if (!need_check) { - _ = try sema.analyzeCall(block, return_err_fn, src, src, .never_inline, false, &args, null); + _ = try sema.analyzeCall(block, return_err_fn, src, src, .never_inline, false, false, &args, null); _ = try block.addUnOp(ret_tag, operand); return always_noreturn; } @@ -16152,7 +16190,7 @@ fn retWithErrTracing( var else_block = block.makeSubBlock(); defer else_block.instructions.deinit(gpa); - _ = try sema.analyzeCall(&else_block, return_err_fn, src, src, .never_inline, false, &args, null); + _ = try sema.analyzeCall(&else_block, return_err_fn, src, src, .never_inline, false, false, &args, null); _ = try else_block.addUnOp(ret_tag, operand); try sema.air_extra.ensureUnusedCapacity(gpa, @typeInfo(Air.CondBr).Struct.fields.len + @@ -20369,7 +20407,7 @@ fn zirBuiltinCall(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError } } const ensure_result_used = extra.flags.ensure_result_used; - return sema.analyzeCall(block, func, func_src, call_src, modifier, ensure_result_used, resolved_args, bound_arg_src); + return sema.analyzeCall(block, func, func_src, call_src, modifier, ensure_result_used, false, resolved_args, bound_arg_src); } fn zirFieldParentPtr(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!Air.Inst.Ref { @@ -21803,7 +21841,7 @@ fn panicWithMsg( Value.@"null", ); const args: [3]Air.Inst.Ref = .{ msg_inst, null_stack_trace, .null_value }; - _ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, &args, null); + _ = try sema.analyzeCall(block, panic_fn, src, src, .auto, false, false, &args, null); return always_noreturn; } @@ -21844,7 +21882,7 @@ fn panicUnwrapError( const err = try fail_block.addTyOp(unwrap_err_tag, Type.anyerror, operand); const err_return_trace = try sema.getErrorReturnTrace(&fail_block, src); const args: [2]Air.Inst.Ref = .{ err_return_trace, err }; - _ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, &args, null); + _ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, false, &args, null); } } try sema.addSafetyCheckExtra(parent_block, ok, &fail_block); @@ -21885,7 +21923,7 @@ fn panicIndexOutOfBounds( } else { const panic_fn = try sema.getBuiltin(&fail_block, src, "panicOutOfBounds"); const args: [2]Air.Inst.Ref = .{ index, len }; - _ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, &args, null); + _ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, false, &args, null); } } try sema.addSafetyCheckExtra(parent_block, ok, &fail_block); @@ -21927,7 +21965,7 @@ fn panicSentinelMismatch( else { const panic_fn = try sema.getBuiltin(parent_block, src, "checkNonScalarSentinel"); const args: [2]Air.Inst.Ref = .{ expected_sentinel, actual_sentinel }; - _ = try sema.analyzeCall(parent_block, panic_fn, src, src, .auto, false, &args, null); + _ = try sema.analyzeCall(parent_block, panic_fn, src, src, .auto, false, false, &args, null); return; }; const gpa = sema.gpa; @@ -21956,7 +21994,7 @@ fn panicSentinelMismatch( } else { const panic_fn = try sema.getBuiltin(&fail_block, src, "panicSentinelMismatch"); const args: [2]Air.Inst.Ref = .{ expected_sentinel, actual_sentinel }; - _ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, &args, null); + _ = try sema.analyzeCall(&fail_block, panic_fn, src, src, .auto, false, false, &args, null); } } try sema.addSafetyCheckExtra(parent_block, ok, &fail_block); diff --git a/src/Zir.zig b/src/Zir.zig index f08d78f8f24a27a02bfe568aa2bb974a8ccc9fc0..d616ea6d148e98be369bbadd017a3acb0e64ba0d 100644 --- a/src/Zir.zig +++ b/src/Zir.zig @@ -2825,10 +2825,11 @@ pub const Inst = struct { pub const Flags = packed struct { /// std.builtin.CallOptions.Modifier in packed form pub const PackedModifier = u3; - pub const PackedArgsLen = u28; + pub const PackedArgsLen = u27; packed_modifier: PackedModifier, ensure_result_used: bool = false, + pop_error_return_trace: bool, args_len: PackedArgsLen, comptime { diff --git a/test/stack_traces.zig b/test/stack_traces.zig index 3b8cc8a6cd585d93de08cc76f7da54852d0e3020..1494f8610427399e0bbed1ec7927fafe48675576 100644 --- a/test/stack_traces.zig +++ b/test/stack_traces.zig @@ -208,6 +208,58 @@ pub fn addCases(cases: *tests.StackTracesContext) void { }, }); + cases.addCase(.{ + .name = "stored errors do not contribute to error trace", + .source = + \\fn foo() !void { + \\ return error.TheSkyIsFalling; + \\} + \\ + \\pub fn main() !void { + \\ // Once an error is stored in a variable, it is popped from the trace + \\ var x = foo(); + \\ x = {}; + \\ + \\ // As a result, this error trace will still be clean + \\ return error.SomethingUnrelatedWentWrong; + \\} + , + .Debug = .{ + .expect = + \\error: SomethingUnrelatedWentWrong + \\source.zig:11:5: [address] in main (test) + \\ return error.SomethingUnrelatedWentWrong; + \\ ^ + \\ + , + }, + .ReleaseSafe = .{ + .exclude_os = .{ + .windows, // TODO + .linux, // defeated by aggressive inlining + }, + .expect = + \\error: SomethingUnrelatedWentWrong + \\source.zig:11:5: [address] in [function] + \\ return error.SomethingUnrelatedWentWrong; + \\ ^ + \\ + , + }, + .ReleaseFast = .{ + .expect = + \\error: SomethingUnrelatedWentWrong + \\ + , + }, + .ReleaseSmall = .{ + .expect = + \\error: SomethingUnrelatedWentWrong + \\ + , + }, + }); + cases.addCase(.{ .name = "try return from within catch", .source = -- 2.54.0