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

libstd: do not assert if allocs are within std.c.max_align_t in raw_c_allocator #8835

Closed
wants to merge 2 commits into from

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented May 19, 2021

After landing #8554, stage2 compiler is unable to build a simple "Hello, World" on my M1 Mac tripping the following assertion in rawCAlloc:

extern "c" fn write(usize, usize, usize) usize;
extern "c" fn exit(usize) noreturn;

pub export fn main() noreturn {
    print();

    exit(0);
}

fn print() void {
    const msg = @ptrToInt("Hello, World!\n");
    const len = 14;
    _ = write(1, msg, len);
}
❯ ../../zig-out/bin/zig build-exe hello.zig
thread 2795147 panic: reached unreachable code
/Users/kubkon/dev/zig/build/lib/zig/std/debug.zig:226:14: 0x104936893 in std.debug.assert (zig)
    if (!ok) unreachable; // assertion failure
             ^
/Users/kubkon/dev/zig/build/lib/zig/std/heap.zig:178:11: 0x1049366ab in std.heap.rawCAlloc (zig)
    assert(ptr_align <= @alignOf(std.c.max_align_t));
          ^
/Users/kubkon/dev/zig/build/lib/zig/std/mem/Allocator.zig:290:40: 0x104a3be2f in std.mem.Allocator.allocAdvancedWithRetAddr (zig)
    const byte_slice = try self.allocFn(self, byte_count, a, len_align, return_address);
                                       ^
/Users/kubkon/dev/zig/build/lib/zig/std/mem/Allocator.zig:159:52: 0x104a37b27 in std.mem.Allocator.create (zig)
    const slice = try self.allocAdvancedWithRetAddr(T, null, 1, .exact, @returnAddress());
                                                   ^
/Users/kubkon/dev/zig/src/Module.zig:3165:36: 0x104a3281f in Module.importPkg (zig)
    const new_file = try gpa.create(Scope.File);
                                   ^
/Users/kubkon/dev/zig/src/Compilation.zig:1605:37: 0x104a3034f in Compilation.update (zig)
            _ = try module.importPkg(module.root_pkg, std_pkg);
                                    ^
/Users/kubkon/dev/zig/src/main.zig:2233:20: 0x1049c0157 in updateModule (zig)
    try comp.update();
                   ^
/Users/kubkon/dev/zig/src/main.zig:1965:17: 0x10495482f in buildOutputType (zig)
    updateModule(gpa, comp, hook) catch |err| switch (err) {
                ^
/Users/kubkon/dev/zig/src/main.zig:186:31: 0x10494276f in mainArgs (zig)
        return buildOutputType(gpa, arena, args, .{ .build = .Exe });
                              ^
/Users/kubkon/dev/zig/src/main.zig:141:20: 0x104941ba3 in main (zig)
    return mainArgs(gpa, arena, args);
                   ^
/Users/kubkon/dev/zig/build/lib/zig/std/start.zig:458:37: 0x104cbc28f in std.start.callMain (zig)
            const result = root.main() catch |err| {
                                    ^
/Users/kubkon/dev/zig/build/lib/zig/std/start.zig:400:12: 0x104943bc7 in std.start.callMainWithArgs (zig)
    return @call(.{ .modifier = .always_inline }, callMain, .{});
           ^
/Users/kubkon/dev/zig/build/lib/zig/std/start.zig:370:12: 0x104943af7 in std.start.main (zig)
    return @call(.{ .modifier = .always_inline }, callMainWithArgs, .{ @intCast(usize, c_argc), c_argv, envp });
           ^
???:?:?: 0x19ac7544f in ??? (???)
Panicked during a panic. Aborting.
[1]    19416 abort      ../../zig-out/bin/zig build-exe hello.zig

I believe this assertion is wrong and should be removed (as I've done in this PR) since it only really covers scalar types:

According to cppreference on max_align_t, its alignment is at least as large as that of every scalar type. On arm64 macOS, max_align_t == c_longdouble which then implies @alignOf(c_longdouble) == 8.

In this particular case, @alignOf(Scope.File) == 16 since Scope.File contains an i128 as a member.


Out of curiosity, I have verified that the same behaviour is observed with C++. That is, for the following source:

#include <iostream>
#include <cstddef>

int main()
{
  std::cout << alignof(__int128_t) << " " << alignof(std::max_align_t) << '\n';
}

we get

> clang -Wl,-syslibroot $(xcrun --show-sdk-path) -lSystem -lc++ -std=c++14 cpp.cpp
> a.out
16 8

which matches Zig's behaviour.


If anyone has got access to an M1, I'd be very grateful if you could verify running this example against stage2 built with master branch and check if you see the same reported behaviour.

@kubkon kubkon requested a review from andrewrk May 19, 2021 11:01
@kubkon kubkon added the standard library This issue involves writing Zig code for the standard library. label May 19, 2021
@LemonBoy
Copy link
Contributor

You have to consider max_align_t from the C standpoint, where f128 and i128 are non-standard extensions. The maximum alignment is given by a c_longdouble that, from what I can see, is defined as a f128 when targeting Linux but not when targeting macOS [see here].

Malloc is meant to respect that alignment or, what many allocator do, provide a higher one. But we can't detect this (beside hardcoding a libc-specific whitelist), that's why you're hitting that assertion.

As I've already stated multiple times (see #6385 and #6674) use c_allocator, that's the best solution.

Side note, the c_longdouble definition should be updated to reflect the target ABI for AArch64.

@daurnimator
Copy link
Contributor

I think the assert is correct: malloc only guarantees suitable alignment for fundamental types (e.g. long long): its just implementation details and luck for any higher alignment.

@kubkon
Copy link
Member Author

kubkon commented May 19, 2021

Hmm, ok, that makes sense - thanks for the input @LemonBoy and @daurnimator. But the issue remains that stage2 is unusable on my M1. @andrewrk, do you think we could use c_allocator rather than raw_c_allocator in Module.zig? This should hopefully fix the issue.

@andrewrk andrewrk closed this in bfd051a May 19, 2021
@Vexu Vexu deleted the raw_c_alloc branch May 19, 2021 17:29
@Vexu Vexu restored the raw_c_alloc branch May 19, 2021 17:30
Snektron pushed a commit to Snektron/zig that referenced this pull request May 19, 2021
when the raw C allocator alignment is not sufficient.

closes ziglang#8835
@kubkon kubkon deleted the raw_c_alloc branch June 29, 2021 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants