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

make the alignment of >= 128 bit integer types 16 bytes to prevent failure to properly align u128 when using 16-bit cmpchxg #2987

Closed
andrewrk opened this issue Aug 1, 2019 · 3 comments
Labels
arch-x86_64 64-bit x86 enhancement Solving this issue will likely involve adding new logic or components to the codebase. stage1 The process of building from source via WebAssembly and the C backend.
Milestone

Comments

@andrewrk
Copy link
Member

andrewrk commented Aug 1, 2019

@cmpxchg on x86_64 supports u128 integers. However @alignOf(u128) reports 8, and without align(16) on the u128, the code segfaults at runtime. This alignment is reported by LLVM as the "ABI alignment". Is this alignment incorrect? Is the atomic alignment requirement higher? Investigation is needed.

Here's the test case:

test "128-bit cmpxchg" {
    if (builtin.arch != .x86_64) {
        return error.SkipZigTest;
    }
    var x: u128 = 1234; // segfault at runtime can be fixed by adding align(16) here
    if (@cmpxchgWeak(u128, &x, 99, 5678, .SeqCst, .SeqCst)) |x1| {
        expect(x1 == 1234);
    } else {
        @panic("cmpxchg should have failed");
    }

    while (@cmpxchgWeak(u128, &x, 1234, 5678, .SeqCst, .SeqCst)) |x1| {
        expect(x1 == 1234);
    }
    expect(x == 5678);

    expect(@cmpxchgStrong(u128, &x, 5678, 42, .SeqCst, .SeqCst) == null);
    expect(x == 42);
}
@andrewrk andrewrk added this to the 0.6.0 milestone Aug 1, 2019
@daurnimator daurnimator added the arch-x86_64 64-bit x86 label Aug 1, 2019
@gereeter
Copy link
Contributor

gereeter commented Aug 1, 2019

See also rust-lang/rust#54341 describing the same issue. As mentioned there, this was fixed in LLVM but reverted. Clang manually hardcodes the alignment of 128 bit types, ignoring what LLVM reports. As brought up in that LLVM change, all investigated ABIs mandated that 128-bit types be aligned to 16 bytes.

@andrewrk andrewrk added contributor friendly This issue is limited in scope and/or knowledge of Zig internals. enhancement Solving this issue will likely involve adding new logic or components to the codebase. stage1 The process of building from source via WebAssembly and the C backend. labels Aug 1, 2019
@andrewrk andrewrk changed the title failure to properly align u128 when using 16-bit cmpchxg make the alignment of >= 128 bit integer types 16 bytes to prevent failure to properly align u128 when using 16-bit cmpchxg Aug 1, 2019
@andrewrk
Copy link
Member Author

andrewrk commented Aug 1, 2019

Thanks for that @gereeter. This is now a contributor friendly issue.

Here is where the alignment of integer types is set:

entry->abi_align = LLVMABIAlignmentOfType(g->target_data_ref, entry->llvm_type);

@andrewrk andrewrk removed the contributor friendly This issue is limited in scope and/or knowledge of Zig internals. label Aug 4, 2019
@andrewrk
Copy link
Member Author

Will be fixed with the merge of #3033

@andrewrk andrewrk modified the milestones: 0.6.0, 0.5.0 Sep 29, 2019
andrewrk added a commit that referenced this issue May 5, 2022
These targets now have a similar disagreement with LLVM about the
alignment of 128-bit integers as x86_64:
 * riscv64
 * powerpc64
 * powerpc64le
 * mips64
 * mips64el
 * sparcv9

See #2987
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-x86_64 64-bit x86 enhancement Solving this issue will likely involve adding new logic or components to the codebase. stage1 The process of building from source via WebAssembly and the C backend.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants