-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: Limit scope of undefined
values (and introduce automatic coercion on function boundaries)
#3328
Comments
How would
|
This definitely needs to be solved, but I'm not sure that validating on function boundaries is enough. Consider this code: const Pun = extern enum {
integer: usize,
pointer: *u32,
};
fn bar(p: *Pun) void {
p.* = Pun{ .integer = 0 };
}
fn foo() *u32 {
var p: Pun = undefined;
bar(&p);
return p.pointer;
} Here, in Thinking more about pointers, it's also important to consider that they can have defined values but would cause a segfault if dereferenced. So it's never safe to perform a fixup through a pointer unless the code is already reading from the pointer. Because of this, the compiler can't just generate checks at every function boundary but must generate them at every read. In some cases, if the optimizer can prove that the memory wasn't altered, it may be able to remove some of these checks. But in the general case that's difficult because of pointer aliasing. The only "leak" I'm aware of for this problem is the But we also have the problem of type punning between const Pun = extern union {
nullable: ?*u32,
nonnull: *u32,
};
fn foo() void {
var p: Pun = .{ .nonnull = undefined };
if (p.nullable == null) {
// is this well defined?
}
} |
(quick recap, probably unnecessary)
(actual comment) So, to clarify my proposal with my understanding of / suggestion in your examples:
Note that whatever validation boundary we choose, be it functions or expressions, basically sets the baseline performance gain of While writing this I got a clear (and status-quo-actionable) idea for |
Magic values being generated at magic times is too difficult a mental model for me. Doesn't ZenOfZig imply I should be the one generating the values, explicitly, where and when I want? I see this as two different problems, both solvable at compile-time:
@SpexGuy, re your => The compiler will not allow primitive types with value constraints (not all possible bit permutations that fit into @sizeof(T) are possible for T) in an (Maybe the C to Zig transpiler needs to generate extern unions that would violate such constraints, but maybe they are fine for all C types?) So no What about As for What is the value of a var of type T in state (Currently) it does not even obey the loosest possible interpretation of value: I think this is most concisely demonstrated by @vi's Or less concisely by this snippet in --relase-fast: const std = @import("std");
pub fn main() !void {
var v1 : u32 = 2;
std.debug.warn("defined v1 is = {}\n",.{v1});
var v2: i32 = undefined;
if(v2 >= 1){
std.debug.warn("undefined v2 is >=1: {}\n",.{v2});
} else if(v2 < 1){
std.debug.warn("undefined v2 is < 1: {}\n",.{v2});
} else {
std.debug.warn("undefined v2 is ???: {}\n",.{v2});
}
}
// ==> using release-fast this prints: undefined v2 is ???: 2
// ==> using relase-safe this prints: undefined v2 is < 1: -1431655766 So when thought of as a value, Why not redefine
This would make it easier to reason about. And allow a simplification of the language: you could remove var undefinedvar : u32 = undefined; // with proposed change: not allowed to use until you have defined it
var uninitializedvar: u32; // currently illegal, why not allow and require a define before use (provable at compile-time) Maybe a new concept "any legal value for that type, I don't care which one, let the compiler choose the fastest one right now" requiring a new keyword var initializedvar : i32 = anylegalval; // compiler, initialize this for me as fast as possible The compiler knows that for an i32 it is a noop to choose anylegalval, but for a u32 it needs to ensure >= 0, it can either refuse to do anything for a non-null ptr (although any non-zero val is legal, even if it segfaults), ... Et voilà, KISS, no nonnull_or_undefined, and no hidden runtime magic. What about (from the documentation): var answer: T = undefined;
return if (@addWithOverflow(T, a, b, &answer)) error.Overflow else answer; Ok, maybe the |
Here is a real world bug caused by the status quo design flaw of Zig: #6790 You can see this results in tempting people to hack around the issue: #6825 The Zig code is: export fn stage2_fetch_file(
stage1: *Module,
path_ptr: [*]const u8,
path_len: usize,
result_len: *usize,
) ?[*]const u8 {
const comp = @intToPtr(*Compilation, stage1.userdata);
const file_path = path_ptr[0..path_len];
const max_file_size = std.math.maxInt(u32);
const contents = comp.stage1_cache_manifest.addFilePostFetch(file_path, max_file_size) catch return null;
result_len.* = contents.len;
return contents.ptr;
} The C++ code is: Error file_fetch(CodeGen *g, Buf *resolved_path, Buf *contents_buf) {
size_t len;
const char *contents = stage2_fetch_file(&g->stage1, buf_ptr(resolved_path), buf_len(resolved_path), &len);
if (contents == nullptr)
return ErrorFileNotFound;
buf_init_from_mem(contents_buf, contents, len);
return ErrorNone;
} Can you spot the bug? When zig loads the file, and the length is 0, it uses |
This itself might be the problem here, we could make |
Well I knew it was a hack lol, but I'm still confused what is going on here if someone want's to clarify... Isn't zig using null and not undefined here? I thought null was a known value of 0? |
@frmdstryr When |
@frmdstryr oh I knew you knew it was a hack, I'm sorry - I didn't mean to throw shade. I'm saying it's not your fault, the language has a flaw. |
Beginner's reaction: why are slices not defined to have optional pointers ( |
@cajw1 One reason I can think of is that we already have a way to express a slice without contents, by setting its |
@rohlem I think you would only have to check one, either As an aside: maybe there is a more general problem/temptation to use undefined instead of either invalid or optional. |
I should probably take this onto IRC instead of taking derailing this thread... but in #1947 it says:
So if I understand this correctly... the reason the slice with 0 len becomes undefined is because the compiler decides it can just optimize out reading the ptr and just leave whatever at the result location before in memory? Can this occur if running with debug/release-safe? |
@frmdstryr Without reading the implementation of |
The decision made in #1831 holds here as well. In terms of guarantees in release-fast mode, undefined is a special value, and coercion from |
I don't see it as a duplicate, but I agree that the decision as I understand it rejects this proposal, so I'm fine it being closed. |
This was originally going to be a response to #3325, but it's more general in scope. So I thought I'd comment on #1947 , but that is more of a tracking issue rather than an open question. So I think a new issue is the best fit, but feel free to close it if it seems too similar.
If you're unfamiliar, please read #1947 for details on
undefined
values (and when they trigger potentially-checked UB), also see the test scenario fixed by 34d02f2 for an example of howundefined
is currently "unsafe". (expl: the cast(?*T)(*T)
is implemented as a no-op, so if*T
isundefined
, the bit pattern may remain0
=> result innull
.)Motivation
As I see it, types are generally useful to readers in that they expose certain invariants about values:
i3
only permits the range[-4, 3]
default
in a switch that covers all cases)Within this formal model of program behaviour, non-conforming and
undefined
values (further shortened to "invalid" values) don't really exist (at least I'd prefer if they didn't). But as an optimization step, and ergonomic feature for uncovering program logic errors, it still makes sense for Zig to expose the concept ofundefined
, never-to-be-used values.The issue is then, in my eyes, this optimization leaking into the program logic, f.e. by (probably non-exhaustive):
Opinion/Discussion
IMO it would be helpful to have (more granular) well-defined boundaries establishing where exactly these invariants hold.
The precedent set by
@ptrCast
of0
to a non-optional*T
, and Zig's Zen to handle errors and edge cases, suggests that a detected invalid value should then trigger a safety panic in safe build modes (/ under scopes with runtime safety enabled).side? note: The type system might even currently be an obstacle, as I wouldn't be surprised if f.e. a comparison
u4 > 15
were optimized out, even if during runtime such an invalid value can manifest. Maybe a builtin@isValid
, or somestd.meta
function for handling these edge cases could improve the situation.If we accept this as "wontfix", then that implies that the implementation of an opt-ptr argument
extern fn(x: ?*c_void)
could be safer than non-opt-ptr argumentextern fn(x: *c_void)
, because it can check fornull
. (Although, with pointers, there are still enough "valid" addresses in unmapped memory you cannot check so easily.)Proposal
As functions seem to me a sensible unit of program analysis, I propose for every function call (and return) to introduce a "coercion" step to replace every "invalid" argument value (and return value) with a "valid" fallback (which is still semantically
undefined
, as far as analysis and runtime safety checks are concerned). (I'm undecided aboutinline
functions, it seems likely that an argument could be made either way.)Thereby function calls become a "validation boundary"; every function can assume its arguments are valid. (And every caller can assume the return value is valid.)
(Note: Doing this at every function call might sound too expensive, but since every function's inputs are now always "valid", it effectively only applies to functions which can introduce invalid values, mainly via keyword
undefined
, and only to affected variables, which partially reflects an already-accepted proposed step of analysis.)EDIT3: As @SpexGuy pointed out below, indirection through a pointer also requires this validation step, and if we don't want to track all pointers written to within a function, this means validation on every write of an invalid (because locally-undefined) value through a pointer (i.e. ensuring that we write a valid-but-undefined value).
For extern Zig functions (that can be called externally, "from C" / dynamically linked etc.), it would be useful to either provide a wrapper function factory in
std.meta
, or (if that is somehow infeasible) have a language-generated assertion as part of the function prologue, checking that Zig's type invariants hold for the given arguments. Note the first would be opt-in, so could be considered less safe. (The same applies to extern functions' return values, which would require this caller-side.)Following this proposal, (non-extern) callers would guarantee that non-optional pointers are non-null, solving #3325 for function arguments (although I don't mean to argue against a
nonnull_or_undefined
attribute, for release build modes). Local variables and local "sources of undefined values" still need to be tracked in an analysis-step, as already proposed in #211 .EDIT2: While this proposal essentially talks about "safety" features (as correct programs should never observably access undefined values), I do think the idea behind it might still end up influencing optimized build modes.
Specifically, it may be interesting to opt-in to this behaviour via a function-scope builtin like a more specialized
@setRuntimeSafety
, i.e.@enableArgumentValidityCheck
.The text was updated successfully, but these errors were encountered: