Missing mask of bits in implicit cast

[daurnimator]

Looking into the CI failure of #2843 I arrived at the following reduced test case:

const builtin = @import("builtin");
const std = @import("std");

fn hashFunc(out: []u8, Ki: usize, in: []const u8) void {
    for (out) |*p, i| {
        p.* = 0xff;
    }
}

test "repro" {
    const Io = std.packed_int_array.PackedIntIo(u1, builtin.endian);
    const Index = std.math.IntFittingRange(0, 1024 - 1);
    const data: [128]u8 = [_]u8{0} ** 128;

    var K_th_bit: Index = undefined;
    hashFunc(std.mem.asBytes(&K_th_bit), 1, "foo");

    std.debug.warn("get {}\n", K_th_bit);
    _ = Io.get(data, K_th_bit, 0);
}

With this applied to make things more obvious:

diff --git a/std/packed_int_array.zig b/std/packed_int_array.zig
index 5cbab2d33..23e64ae6b 100644
--- a/std/packed_int_array.zig
+++ b/std/packed_int_array.zig
@@ -46,9 +46,10 @@ pub fn PackedIntIo(comptime Int: type, comptime endian: builtin.Endian) type {
         pub fn get(bytes: []const u8, index: usize, bit_offset: u7) Int {
             if (int_bits == 0) return 0;
 
+            std.debug.warn("index={} int_bits={} bit_offset={}\n", index, usize(int_bits), bit_offset);
             const bit_index = (index * int_bits) + bit_offset;
             const max_end_byte = (bit_index + max_io_bits) / 8;
-
+            std.debug.warn("bit_index={} max_end_byte={}\n", bit_index, max_end_byte);
             //Using the larger container size will potentially read out of bounds
             if (max_end_byte > bytes.len) return getBits(bytes, MinIo, bit_index);
             return getBits(bytes, MaxIo, bit_index);
@@ -63,6 +64,7 @@ pub fn PackedIntIo(comptime Int: type, comptime endian: builtin.Endian) type {
             const tail_keep_bits = container_bits - (int_bits + head_keep_bits);
 
             //read bytes as container
+            std.debug.warn("start_byte={} bytes.len={}\n", start_byte, bytes.len);
             const value_ptr = @ptrCast(*align(1) const Container, &bytes[start_byte]);
             var value = value_ptr.*;

$ bin/zig test ../bug.zig --test-filter repro 
1/1 test "repro"...get 1023
index=65535 int_bits=1 bit_offset=0
bit_index=65535 max_end_byte=8192
start_byte=8191 bytes.len=128
index out of bounds
/home/daurnimator/src/zig/build/lib/zig/std/packed_int_array.zig:68:73: 0x20573c in std.packed_int_array.PackedIntIo(u1,builtin.Endian.Little).getBits (test)
            const value_ptr = @ptrCast(*align(1) const Container, &bytes[start_byte]);
                                                                        ^
/home/daurnimator/src/zig/build/lib/zig/std/packed_int_array.zig:54:57: 0x2049b9 in std.packed_int_array.PackedIntIo(u1,builtin.Endian.Little).get (test)
            if (max_end_byte > bytes.len) return getBits(bytes, MinIo, bit_index);
                                                        ^
/home/daurnimator/src/zig/bug.zig:19:15: 0x204674 in test "repro" (test)
    _ = Io.get(data, K_th_bit, 0);
              ^
/home/daurnimator/src/zig/build/lib/zig/std/special/test_runner.zig:13:25: 0x2272ed in std.special.main (test)
        if (test_fn.func()) |_| {
                        ^
/home/daurnimator/src/zig/build/lib/zig/std/special/start.zig:146:22: 0x226156 in std.special.posixCallMainAndExit (test)
            root.main() catch |err| {
                     ^
/home/daurnimator/src/zig/build/lib/zig/std/special/start.zig:66:5: 0x225fd0 in std.special._start (test)
    @noInlineCall(posixCallMainAndExit);
    ^

Tests failed. Use the following command to reproduce the failure:
/home/daurnimator/src/zig/build/zig-cache/o/w7Ko8i5VR8YnH4vrgbC5B9TZkWtf_gBjJ8jlPcIj_StPJj3qSnj8l4shkxzej4Ey/test

Based on how I was able to reduce the test case, it appears as though the call to .get doesn't mask off bits as it implicitly casts from Index to usize.

[daurnimator]

Maybe related to #1512 (comment) ?

[daurnimator]

Simpler test case:

const std = @import("std");

fn hashFunc(out: []u8, in: []const u8) void {
    for (out) |*p, i| {
        p.* = 0xff;
    }
}

fn printUsize(x: usize) usize {
    std.debug.warn("inside function call: {}\n", x);
    return x;
}

pub fn main() void {
    const Index = std.math.IntFittingRange(0, 1024 - 1);

    var K_th_bit: Index = undefined;
    hashFunc(std.mem.asBytes(&K_th_bit), "foo");

    std.debug.warn("outside function call: {}\n", K_th_bit);
    const x = printUsize(K_th_bit);
    std.debug.warn("from function call: {}\n", x);
}

Output:

outside function call: 1023
inside function call: 65535
from function call: 65535

[mikdusan]

here's a reduction that is useful for IR and LLVM-IR production:

zig build-obj bug.zig --verbose-ir --verbose-llvm-ir

fn set_v0(a: *u10) void {
    a.* = 1023;
}   

fn set_v1(a: []u8) void {
    for (a) |*p,i| {
        p.* = 0xff;
    }
}   

export fn foo() void {
    @setRuntimeSafety(false);
    
    var a: u10 = undefined;
    set_v1(@import("std").mem.asBytes(&a));
    
    var b: usize = undefined;
    b = a;
}

const builtin = @import("builtin");
pub fn panic(msg: []const u8, error_return_trace: ?*builtin.StackTrace) noreturn {
    while (true) {}
}

My initial impression is that zext is operating on the i10 as if it were i16 and thus using value 0xffff instead of 0x03ff.

define void @foo() #2 !dbg !35 {
Entry:
  %a = alloca i10, align 2
  %0 = alloca %"[]u8", align 8
  %b = alloca i64, align 8
  call void @llvm.dbg.declare(metadata i10* %a, metadata !39, metadata !DIExpression()), !dbg !43
  %1 = call fastcc [2 x i8]* @std.mem.asBytes(i10* %a), !dbg !44
  %2 = getelementptr inbounds %"[]u8", %"[]u8"* %0, i32 0, i32 0, !dbg !44
  %3 = getelementptr inbounds [2 x i8], [2 x i8]* %1, i64 0, i64 0, !dbg !44
  store i8* %3, i8** %2, !dbg !44
  %4 = getelementptr inbounds %"[]u8", %"[]u8"* %0, i32 0, i32 1, !dbg !44
  store i64 2, i64* %4, !dbg !44
  call fastcc void @set_v1(%"[]u8"* %0), !dbg !45
  call void @llvm.dbg.declare(metadata i64* %b, metadata !42, metadata !DIExpression()), !dbg !46
  %5 = load i10, i10* %a, align 2, !dbg !47
  %6 = zext i10 %5 to i64, !dbg !47
  store i64 %6, i64* %b, align 8, !dbg !47
  ret void, !dbg !48
}

Here's x86_64 asm showing use of movzwl Move Zero-Extended Word to Long without regard to any non-byte-aligned integers:

_foo:
Lfunc_begin1:
    .loc    1 11 0
    .cfi_startproc
    pushq   %rbp
    .cfi_def_cfa_offset 16
    .cfi_offset %rbp, -16
    movq    %rsp, %rbp
    .cfi_def_cfa_register %rbp
    subq    $32, %rsp
    leaq    -2(%rbp), %rdi
Ltmp2:
    .loc    1 15 38 prologue_end
    callq   _std.mem.asBytes
    movq    %rax, -24(%rbp)
    movq    $2, -16(%rbp)
    leaq    -24(%rbp), %rdi
    .loc    1 15 11 is_stmt 0
    callq   _set_v1
    .loc    1 18 9 is_stmt 1
    movzwl  -2(%rbp), %ecx
    movl    %ecx, %eax
    movq    %rax, -32(%rbp)
    .loc    1 11 22
    addq    $32, %rsp
    popq    %rbp
    retq
Ltmp3:
Lfunc_end1:
    .cfi_endproc

[mikdusan]

transcript of related IRC discussion:

daurnimator: okay fun.... maybe this is an LLVM bug rather than a zig bug? I see a %17 = zext i10 %16 to i64, !dbg !1420 in there.... my working theory for the bug was that that sort of zext was missing in there
daurnimator: ==> the result of a zext on an i10 is ending up as 65535. which is above the max of 1023 you would expect
mikdusan: another hint: x86_64 ends up as movzwl Move Zero-Extended Word to Long, without regard to our i10 size
andrewrk: i10 is represented by llvm as an i16
andrewrk: so at the zext instruction, the unused bits of the 2 bytes are assumed to be zeroes
andrewrk: if you ask llvm what the "store size" of i10 is, the answer is 2
mikdusan: i don't think llvm assumes it's zero. it just takes whatever value happens to be there and performs zext i1
andrewrk: but I believe the result is undefined if the unused bits are not zero
mikdusan: does this mean zig will have to explicitly mask?
andrewrk: this code does a pointer reinterpretation and then does not correctly set the bits of the reinterpreted memory
andrewrk: that means the value of the i10 is undefined
andrewrk: setting all bits of an integer to 1 produces a well-defined value only when the integer is byte-aligned
andrewrk: zig would be allowed to, for example, use the extra bit from a u15 to indicate that the integer is undefined
andrewrk: ultimately, the thing that is happening here is that u10 has a "bit pattern" that is not being respected by the pointer reinterpretation

[andrewrk]

This is working as designed.

@sizeOf(u10) == 2. There is a range of bit patterns within those 2 bytes that represent a valid i10 value. The other bit patterns represent undefined. One such undefined bit pattern is 0xffff which is what this code sets the bits to.

In this code I have inserted comments:

    var a: u10 = undefined;
    // Here a is undefined
    set_v1(@import("std").mem.asBytes(&a));
    // Here a is undefined
    
    var b: usize = undefined;
    // Here b is undefined
    b = a;
    // Here b is undefined