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

std: Apple Silicon: no fstat$INODE64 symbol found #6842

Merged
merged 2 commits into from
Oct 28, 2020

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Oct 27, 2020

It seems that Apple has finally got rid of the 32bit versions of
fstat and fstatat, and instead, only 64bit versions are available
on BigSur and Apple Silicon.

The tweak in this commit is required to make Zig stage1 compile on
BigSur + aarch64.

@kubkon kubkon changed the title stage1: Apple Silicon: no fstat$INODE64 symbol found std: Apple Silicon: no fstat$INODE64 symbol found Oct 27, 2020
@kubkon kubkon added standard library This issue involves writing Zig code for the standard library. os-macos arch-aarch64 64-bit ARM labels Oct 27, 2020
It seems that Apple has finally got rid of the 32bit versions of
`fstat` and `fstatat`, and instead, only 64bit versions are available
on BigSur and Apple Silicon.

The tweak in this commit is required to make Zig stage1 compile on
BigSur + aarch64.
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting stuff.

lib/std/c.zig Outdated
Comment on lines 127 to 134
pub usingnamespace switch (builtin.arch) {
.aarch64 => struct {
pub extern "c" fn fstatat(dirfd: fd_t, path: [*:0]const u8, stat_buf: *Stat, flags: u32) c_int;
},
else => struct {
pub const fstatat = @"fstatat$INODE64";
},
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it maybe be cleaner to simply list both functions and rely on lazy evaluation?

pub const fstatat = switch (builtin.arch) {
    .aarch64 => fstatat,
    else => @"fstatat$INODE64",
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent suggestion!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, any ideas if there's a way to avoid overriding the previous definition, i.e., fstatat has already been defined by pub extern "c" fn fstatat...?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a step back and reorganize things. Is the previous definition correct already? Can just use that. If not, sounds like fstatat needs to become one of the functions that moves from the common file to OS-specific files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to keep all the os-related conditional definitions/renames in the switch below.

Hmm, any ideas if there's a way to avoid overriding the previous definition, i.e., fstatat has already been defined by pub extern "c" fn fstatat...?

You can handle it the same way fstat is handled below: define fstatat in c/darwin.zig together with its weird cousin fstatat$INODE64 (and add a comment explaining why it's defined there) and then export the correct fstat symbol below. Other platforms get the usual pub extern "c" fn fstatat(..) ..., it's a bit repetitive but I couldn't figure out a nicer looking way to handle this symbol mess.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 4f50958. Have a look and lemme know if that's what you had in mind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Perhaps it's time to think of moving all the c definitions in a separate file for each os.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Orrr... we could resurrect Andrew's idea of an @extern suggested in #3971 and augment it with a way to specify a comptime string as symbol name. This way we finally get the weak-symbol mechanism and gain a cleaner way to deal with those nasty renames.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, after a quick browse I think I'm partial to the second solution of weak-symbol mechanism. This would indeed make a lot of things cleaner and more elegant.

@jedisct1
Copy link
Contributor

Looks good. Thanks a lot for bringing Zig to Apple Silicon!

@LemonBoy
Copy link
Contributor

Don't forget to update mem.page_size, Apple Silicon has 16k pages (Time to address #2564 too?)

@kubkon
Copy link
Member Author

kubkon commented Oct 28, 2020

Don't forget to update mem.page_size, Apple Silicon has 16k pages (Time to address #2564 too?)

Yep, but I'll make it the subject of a subsequent PR.

@kubkon kubkon merged commit 1a171a1 into ziglang:master Oct 28, 2020
@kubkon kubkon deleted the aarch64-macos-fix branch October 28, 2020 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-aarch64 64-bit ARM os-macos 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.

None yet

4 participants