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

alignedAlloc incorrectly calculates byte count #1851

Closed
tgschultz opened this Issue Dec 21, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@tgschultz
Copy link
Contributor

tgschultz commented Dec 21, 2018

An issue discussed in #1738 was that @sizeOf(T) doesn't account for alignment. alignedAlloc uses @sizeOf(T) without regard for alignment as well. In some cases this will cause an allocator to allocate fewer bytes than are actually used.

Assuming x86 or x64 hardware:

test ""
{
    //@sizeOf(u24) == 3;
    //@alignOf(u24) == 4;
    var x = try debug.global_allocator.alloc(u24, 2); //will be 6 bytes, but should be 8
    x[0] = 0xFFFFFF;
    x[1] = 0xFFFFFF;
    
    //Have to alignCast or we get:
    //error: expected type '*align(4) u8', found '*u8'
    for(@alignCast(1, @sliceToBytes(x))) |*b| b.* = 0x00;
    //All assertions fail
    debug.assert(@sliceToBytes(x).len == 8);
    debug.assert(x[0] == 0x00);
    debug.assert(x[1] == 0x00);
}

It would appear as though both @sliceToBytes and @bytesToSlice are similarly affected.

That thing with having to alignCast for for seems to be an unrelated problem.

@andrewrk andrewrk added the bug label Dec 21, 2018

@andrewrk andrewrk added this to the 0.4.0 milestone Dec 21, 2018

kristate added a commit to kristate/zig that referenced this issue Dec 31, 2018

kristate added a commit to kristate/zig that referenced this issue Jan 3, 2019

kristate added a commit to kristate/zig that referenced this issue Feb 4, 2019

kristate added a commit to kristate/zig that referenced this issue Feb 4, 2019

@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Feb 15, 2019

comptime {
    @compileLog("type", "@sizeOf", "@alignOf", "@sizeOf([1]T)");
    var i = 0;
    while (i < 33) : (i += 1) {
        const T = @IntType(false, i * 8);
        @compileLog(T, @sizeOf(T), @alignOf(T), @sizeOf([1]T));
    }
}
| "type", "@sizeOf", "@alignOf", "@sizeOf([1]T)"
| u0, 0, 0, 0
| u8, 1, 1, 1
| u16, 2, 2, 2
| u24, 3, 4, 4
| u32, 4, 4, 4
| u40, 5, 8, 8
| u48, 6, 8, 8
| u56, 7, 8, 8
| u64, 8, 8, 8
| u72, 9, 8, 16
| u80, 10, 8, 16
| u88, 11, 8, 16
| u96, 12, 8, 16
| u104, 13, 8, 16
| u112, 14, 8, 16
| u120, 15, 8, 16
| u128, 16, 8, 16
| u136, 17, 8, 24
| u144, 18, 8, 24
| u152, 19, 8, 24
| u160, 20, 8, 24
| u168, 21, 8, 24
| u176, 22, 8, 24
| u184, 23, 8, 24
| u192, 24, 8, 24
| u200, 25, 8, 32
| u208, 26, 8, 32
| u216, 27, 8, 32
| u224, 28, 8, 32
| u232, 29, 8, 32
| u240, 30, 8, 32
| u248, 31, 8, 32
| u256, 32, 8, 32

I'm going to solve this by defining @sizeOf to be the last column. So @sizeOf(T) will be guaranteed to be equal to @sizeOf([1]T) and also guarantee @alignOf(T) <= @sizeOfT).

I believe we're actually calling the wrong LLVM function to determine the size of things. Fix coming shortly.

@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Feb 15, 2019

After the fix:

| "type", "@sizeOf", "@alignOf"
| u0, 0, 0
| u8, 1, 1
| u16, 2, 2
| u24, 4, 4
| u32, 4, 4
| u40, 8, 8
| u48, 8, 8
| u56, 8, 8
| u64, 8, 8
| u72, 16, 8
| u80, 16, 8
| u88, 16, 8
| u96, 16, 8
| u104, 16, 8
| u112, 16, 8
| u120, 16, 8
| u128, 16, 8
| u136, 24, 8
| u144, 24, 8
| u152, 24, 8
| u160, 24, 8
| u168, 24, 8
| u176, 24, 8
| u184, 24, 8
| u192, 24, 8
| u200, 32, 8
| u208, 32, 8
| u216, 32, 8
| u224, 32, 8
| u232, 32, 8
| u240, 32, 8
| u248, 32, 8
| u256, 32, 8
@tgschultz

This comment has been minimized.

Copy link
Contributor Author

tgschultz commented Feb 15, 2019

This will break existing code that expects things like @sizeOf(u24) == 3. This is at least relevant to serialize/deserialize in byte-packed mode. I'm not clear on what the fix would be without an equivalent to the current @sizeOf behavior. It also seems like it might be misleading, though that could also be said for the current behavior.

@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Feb 15, 2019

That's correct. Given a [2]T, @sizeOf(T) is defined to be the byte offset between the element at index 0 and the element at index 1.

@sizeOf(u24) is becoming 4, which is how many bytes a u24 takes up as a struct field or an array element (non-packed).

serialize/deserialize were fixed with a 1 line change:

-            const int_size = @sizeOf(U);
+            const int_size = (U.bit_count + 7) / 8;

The docs for @sizeOf contain a new note about integers.

@andrewrk andrewrk closed this in 7293e01 Feb 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.