Skip to content
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

Add mask before truncating dereferenced bit pointers #9584

Merged
merged 2 commits into from Aug 19, 2021

Conversation

Snektron
Copy link
Collaborator

Loading the value of a non-power-of-2 int larger than 8 bits yields something as follows in stage 1:

%1 = load i8, i8* %the_ptr, align 1
%2 = lshr i8 %1, 1
%3 = trunc i8 %2 to i1

Unfortunately, LLVM does not always clear the upper bits of the resulting value after the truncate, as is evident in this snippet:

const std = @import("std");

const A = packed struct {
    a: bool,
    b: bool,
    c: bool,
    d: bool,

    e: bool,
    f: bool,
    g: bool,
    h: bool,
};

const X = union {
    x: A,
    y: u64,
};

pub fn a(
    x0: i32,
    x1: i32,
    x2: i32,
    x3: i32,
    x4: i32,
    flag_a: bool,
    flag_b: bool,
) void {
    @breakpoint();
    _ = x0;
    _ = x1;
    _ = x2;
    _ = x3;
    _ = x4;
    _ = flag_a;
    std.debug.print("{}\n", .{@ptrCast(*const u8, &flag_b).*});
}

pub fn b(x: *X) void {
    a(0, 1, 2, 3, 4, x.x.a, x.x.b);
}

pub fn main() void {
    var flags = A{
        .a = false,
        .b = true,
        .c = false,
        .d = false,

        .e = false,
        .f = true,
        .g = false,
        .h = false,
    };
    var x = X{
        .x = flags,
    };
    b(&x);
}

Above program yields 17, instead of the expected 1. Note that the many arguments are required to force flag_b to be placed on the stack, and the union is also required.

Of course, the upper bits of the result of the truncate are undefined, which would be fine, if llvm didn't try to use them afterwards. With this patch, LLVM is explicitly instructed to generate a masking operation before the truncate. This is also what clang itself seems to do when naively translating above snippet to C.

@andrewrk
Copy link
Member

Needs a new behavior test case please

@Snektron
Copy link
Collaborator Author

Note that this bug only manifests itself in debug mode on my machine. I suspect it might also be different on other architectures. In any case, i think that the extra mask operation justifies the potential future headaches.

@Snektron
Copy link
Collaborator Author

Snektron commented Aug 19, 2021

Oh and by the way, this may seem like an esoteric bug, but i hit it during stage 2 development.

This struct:

zig/src/Zir.zig

Lines 2147 to 2154 in 4c9d417

flags: packed struct {
is_allowzero: bool,
is_mutable: bool,
is_volatile: bool,
has_sentinel: bool,
has_align: bool,
has_bit_range: bool,
_: u2 = undefined,

Is loaded here, causing an incorrect value for is_mutable for example for the type *align(1:2:3) u8:

zig/src/Sema.zig

Lines 6430 to 6432 in 4c9d417

inst_data.flags.is_mutable,
inst_data.flags.is_allowzero,
inst_data.flags.is_volatile,

Which in turn fails this check if both pointers have a different binary value for mutable:

if (info_a.mutable != info_b.mutable)

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to merge provided the tests pass. We should also file an LLVM bug report upstream.

@Snektron
Copy link
Collaborator Author

Also submitted as LLVM issue: https://bugs.llvm.org/show_bug.cgi?id=51529

@andrewrk
Copy link
Member

Brilliant, thank you for the LLVM bug!

@andrewrk andrewrk merged commit 5cd1d42 into ziglang:master Aug 19, 2021
andrewrk added a commit that referenced this pull request Aug 20, 2021
This was a workaround for an LLVM 12 bug which has been fixed in LLVM
13.

This reverts commit 5cd1d42 but keeps
the test case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants