ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
33.86k stars 2.48k forks source link

Provenance related LLVM miscompilation in release mode #20198

Open cryptocode opened 3 months ago

cryptocode commented 3 months ago

Zig Version

0.12.0

Steps to Reproduce and Observed Behavior

@matu3ba mentioned that long-standing pointer provenance related bugs exists in LLVM.

Relevant LLVM issues: https://github.com/llvm/llvm-project/issues/34577 and https://github.com/llvm/llvm-project/issues/33896

I found a C repro by Matthew House. I couldn't find a relevant Zig tracking issue for the miscompilation, so here's a corresponding Zig repro tested on Zig 0.12:

var x: usize = 0;

// Removing export makes the miscompilation go away
// Removing noalias makes the miscompilation go away
pub export fn f(noalias ptr_to_x: *usize) usize {

    // Changing this to const makes the miscompilation go away
    var p = ptr_to_x;
    _ = &p;

    p.* = 1;
    if (p == &x) {
        // Expected branch, taken only in Debug mode
        p.* = 2;
    }
    return p.*;
}

pub fn main() void {
    if (f(&x) == 1) @panic("p != &x");
}

This panics under all the three release modes, but not under debug mode.

Expected Behavior

No panic when compiling in release mode

rohlem commented 3 months ago

To me it looks like the compiler assumes that a pointer value copied from a pointer explicitly marked noalias is unequal to all other pointers (that it is related with, in this case by equality). Why isn't that allowed? noalias isn't documented in the langref yet, so it's possible I'm misunderstanding what exactly we want that attribute to do. In my current understanding this seems reasonable, though it would be nicer to provide a safety-check that triggers in debug mode, unrelated to const-ness of p and export of f. (I somehow doubt that safety checks for aliasing are planned, but I would find them useful.)

tau-dev commented 2 months ago

I somehow doubt that safety checks for aliasing are planned, but I would find them useful.

They are planned! #476. And I'm looking into it right now : )