Skip to content

Commit

Permalink
stage2: Pop error trace when storing error to var/const
Browse files Browse the repository at this point in the history
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 ziglang#1923 (comment)
  • Loading branch information
topolarity committed Sep 14, 2022
1 parent d0ee1bc commit 09842f9
Show file tree
Hide file tree
Showing 5 changed files with 147 additions and 42 deletions.
35 changes: 18 additions & 17 deletions lib/test_runner.zig
Original file line number Diff line number Diff line change
Expand Up @@ -61,23 +61,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", .{});
Expand Down
13 changes: 13 additions & 0 deletions src/AstGen.zig
Original file line number Diff line number Diff line change
Expand Up @@ -8464,9 +8464,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),
},
Expand Down
86 changes: 62 additions & 24 deletions src/Sema.zig
Original file line number Diff line number Diff line change
Expand Up @@ -5515,6 +5515,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;
Expand Down Expand Up @@ -5622,7 +5623,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 {
Expand Down Expand Up @@ -5734,6 +5735,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 {
Expand Down Expand Up @@ -6183,19 +6185,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) {
Expand Down Expand Up @@ -10400,7 +10438,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 => {
Expand All @@ -10411,7 +10449,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: [2]Air.Inst.Ref = .{ msg_inst, err_return_trace };
_ = 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,
Expand Down Expand Up @@ -15549,7 +15587,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;
}
Expand All @@ -15560,7 +15598,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 +
Expand Down Expand Up @@ -19674,7 +19712,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 {
Expand Down Expand Up @@ -21092,7 +21130,7 @@ fn panicWithMsg(
Value.@"null",
);
const args: [2]Air.Inst.Ref = .{ msg_inst, null_stack_trace };
_ = 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;
}

Expand Down Expand Up @@ -21133,7 +21171,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);
Expand Down Expand Up @@ -21174,7 +21212,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);
Expand Down Expand Up @@ -21216,7 +21254,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;
Expand Down Expand Up @@ -21245,7 +21283,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);
Expand Down
3 changes: 2 additions & 1 deletion src/Zir.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2820,10 +2820,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 {
Expand Down
52 changes: 52 additions & 0 deletions test/stack_traces.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down

0 comments on commit 09842f9

Please sign in to comment.