Compiler crash on mem.eql with undefined slice

[DutchGhost]

calling mem.eql with a constant undefined slice, crashes the compiler.

const mem = @import("std").mem;

fn foo() void {
    const n: [] const u8 = undefined;

    comptime {
        if (mem.eql(u8, n, "")) {}
    }
}

test "" {
    comptime foo();
}

interestingly this fails to compile, complaining about the use of an undefined value:

test "" {
    const n: [] const u8 = undefined;

    comptime {
        if (mem.eql(u8, n, "")) {}
    }
}

little backtrace:

unreachable: /deps/zig/src/analyze.cpp:const_values_equal_ptr:5506
Aborted (core dumped)

maybe somewhat related to #1293

[Sahnvour]

There is an easy fix in https://github.com/ziglang/zig/blob/master/src/analyze.cpp#L5807-L5814, return false if any of the fields is undef. This is in the context of fn_eval_eql. What do you think @andrewrk ?

Applying it, the compilation error will actually trigger two times, I assume once when instanciating the function and once when executing it at comptime. Is that okay ?

[andrewrk]

Making const_values_equal return false if any of the fields is undefined might be an okay way to solve this. However, before going down that path, I want to explore what it would look like to uphold what appears to be the existing semantics, which is that const_values_equal should not be called on undefined values. What that looks like is looking up the call stack, and asking ourselves "why is const_values_equal getting called on an undefined value, and is there something that should be changed so that doesn't happen?"

Another thing is to consider the source code and ask, "what is the best thing for the compiler to do, to help the user the most?" Zig should be able to figure out at compile time that this code dereferences an undefined pointer, which is undefined behavior. So whatever our solution is, a test for if it's a good solution is how obvious it is that the compile error tells the user they are dereferencing an undefined pointer.

Looking up the stack with a debugger, I see that it comes from the function fn_eval_eql, and up further,

            auto entry = ira->codegen->memoized_fn_eval_table.maybe_get(exec_scope);

which means that the undefined value is interfering with Zig's ability to cache comptime function calls. Thinking about what should happen with regards to undefined passed as comptime function call arguments, I think it makes sense that if you call foo(undefined) at comptime and then do it again, it can use the same cached value. With this understanding we should be able to produce an even smaller test case:

comptime {
    var x: i32 = undefined;
    foo(x);
}
fn foo(x: i32) void {}

This case gives:

/home/andy/downloads/zig/build/test.zig:3:9: error: use of undefined value here causes undefined behavior
    foo(x);
        ^

So now I'm wondering, why is it an error simply to pass an undefined value to a comptime function call? Shouldn't the error be when the value is dereferenced? This seems related to #1947.

Putting a breakpoint on add_node_error and using a debugger to look up the stack gives me a clue: in ir_analyze_fn_call_inline_arg we have this:

     ConstExprValue *arg_val = ir_resolve_const(ira, casted_arg, UndefBad);

which means that Zig is giving a compile error for parameters that are undefined when doing a comptime function call, but it is not looking recursively into aggregate data types to find undefined values. So this is an inconsistency that we should resolve. Looking at the rules that #1947 specify, there's no reason for undefined parameters to cause undefined behavior. This is actually an incorrect compile error. So now I change UndefBad to UndefOk and run the example again. And so now our new simple example crashes in the same way:

zig: /home/andy/downloads/zig/src/analyze.cpp:4810: uint32_t hash_const_val(ConstExprValue*): Assertion `const_val->special == ConstValSpecialStatic' failed.

This makes sense - it's the same problem noted above where the undefined value is interfering with Zig's ability to memoize the comptime function call.

So now the task is to update the codebase to make fn_eval_eql and fn_eval_hash support undefined values, whether the undefined value is found recursively on a field or not. I can think of an approach to this:

Update const_values_equal semantics to treat undefined as a distinct state. In other words, make it true that undefined == undefined, at least according to const_values_equal. If we do this, we might consider this example:

comptime {
    const a: i32 = undefined;
    const b: i32 = undefined;
    @compileLog(a == b);
}

which currently gives this output:

/home/andy/dev/zig/build/test.zig:4:17: error: use of undefined value here causes undefined behavior
    @compileLog(a == b);
                ^

This behavior is also planned to be changed with #1947. The expected result is bool(undefined). But either way, the point I'm making is that const_values_equal, from the perspective of the == operator, is not meant to encounter undefined values, and that's why those asserts are there.

So if we take this approach, we have to make sure that we do not accidentally propagate this new way of treating undefined values to callers of const_values_equal. Perhaps there would even be a flag passed to the function to indicate the way that undefined values would be treated.

Applying it, the compilation error will actually trigger two times, I assume once when instanciating the function and once when executing it at comptime. Is that okay ?

This is a separate problem, which I would be happy to discuss, but I don't think you should have to worry about it for this issue.

Hope that helps @Sahnvour, let me know if I can provide any more guidance.

[Sahnvour]

I think I get your concerns, but I'm a bit confused about what your final take really is. Assuming you are not entirely satisfied by

Update const_values_equal semantics to treat undefined as a distinct state. In other words, make it true that undefined == undefined, at least according to const_values_equal.

Since undefined is a value allowed in many places of the language, including comptime, keeping the actual semantics of const_values_equal basically means handling undefined values before calling it at every call site. Or at least actively trying to not get in the situation to call it. Either way, this would split something that is conceptually the same thing: comparing values that can be undefined, right ?

[Vexu]

This now gives a proper compile error.

/usr/lib/zig/std/mem.zig:351:10: error: use of undefined value here causes undefined behavior
    if (a.len != b.len) return false;
... etc

No runtime error since #211 hasn't been implemented yet.