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

Update use of async functions in self hosted compiler #3780

Merged
merged 5 commits into from
Nov 27, 2019

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Nov 26, 2019

Follow up to #3748.

Blocked by #3190 (or something similar).
#3777 is avoided with a @panic.

@andrewrk
Copy link
Member

Blocked by #3190 (or something similar).
#3777 is avoided with a @panic.

I'm having a look into these issues today.

@Vexu Vexu force-pushed the stage2-async-review branch from b44baae to 798d05d Compare November 26, 2019 20:11
@Vexu
Copy link
Member Author

Vexu commented Nov 26, 2019

Looks like there is a bug in std.heap.c_allocator. Running zig-cache/bin/zig fmt build.zig crashes with incorrect alignment when using it. direct_allocator works fine though.

@andrewrk
Copy link
Member

andrewrk commented Nov 26, 2019

Currently, c_allocator just wraps realloc. This guarantees an alignment of 8 bytes only. However, an async function frame (on some targets, such as x86_64) requires alignment 16. I briefly tried to use one of the libc aligned memory allocation functions in the past, but it wasn't available on all the libcs we wanted to target.

However, that was before we had libc versioning support, and upon further reflection, we can certainly do better than realloc unconditionally.

Here's some brief research:

  • musl supports memalign, posix_memalign, and aligned_alloc. (we provide musl so it will always have all of these)
  • glibc supports memalign in all versions; posix_memalign since 2.1.91, aligned_alloc since 2.16.
  • FreeBSD and NetBSD support posix_memalign and aligned_alloc
  • macOS supports posix_memalign
  • mingw-w64 and windows UCRT have _aligned_realloc

I believe that covers all the targets, so we can solve this with some additional logic in std.heap.cRealloc and std.heap.cShrink.

@andrewrk
Copy link
Member

I extracted an issue out of my previous comment: #3783

@andrewrk andrewrk merged commit 83c664e into ziglang:master Nov 27, 2019
@Vexu Vexu deleted the stage2-async-review branch November 27, 2019 18:40
@richfelker
Copy link

Per specification, realloc guarantees max_align_t alignment, which should be 16 on x86_64, but does not preserve any further alignment obtained from memalign-family. However max_align_t alignment is only 8 on 32-bit x86 and some other archs. musl always provides at least 16 but this is an implementation detail. Windows may have less than 16 on x86_64 because Windows..

@andrewrk
Copy link
Member

Thanks - I'm going to update #3783 with a recommendation based on this information.

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.

3 participants