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

Fixed struct 'posix.system__struct_6652' has no member named 'MSF' compile error for other OS target #20833

Closed
wants to merge 1 commit into from

Conversation

DanB91
Copy link

@DanB91 DanB91 commented Jul 27, 2024

In my OS project, which has a target of x86_64-other-gnu I am parsing DWARF debug symbols and with the latest pull of 0.14 I am getting this compile error:

/Users/danielbokser/zig/lib/std/posix.zig:80:23: error: struct 'posix.system__struct_6652' has no member named 'MSF'
pub const MSF = system.MSF;
                ~~~~~~^~~~
/Users/danielbokser/zig/lib/std/posix.zig:48:13: note: struct declared here
    else => struct {
            ^~~~~~
referenced by:
    isValidMemory: /Users/danielbokser/zig/lib/std/debug.zig:697:46
    read: /Users/danielbokser/zig/lib/std/debug.zig:767:31
    load__anon_6654: /Users/danielbokser/zig/lib/std/debug.zig:773:31
    readIntChecked__anon_4363: /Users/danielbokser/zig/lib/std/dwarf.zig:2787:20
    readUnitHeader: /Users/danielbokser/zig/lib/std/dwarf.zig:479:59
    scanAllFunctions: /Users/danielbokser/zig/lib/std/dwarf.zig:666:51
    openDwarfDebugInfo: /Users/danielbokser/zig/lib/std/dwarf.zig:2191:28

It seems that on zig/lib/std/debug.zig:697:46 Zig erroneously thinks this a POSIX target based on the have_msync constant, which it is not. This PR attempts to fix this.

Not sure if this is the best fix, but it does fix my compile error.

@@ -844,7 +844,7 @@ pub const StackIterator = struct {

const have_msync = switch (native_os) {
.wasi, .emscripten, .windows => false,
else => true,
else => posix.system.ucontext_t != void,
Copy link
Contributor

@rootbeer rootbeer Jul 28, 2024

Choose a reason for hiding this comment

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

I can see how this fixes your problem, but I don't think msync existence should logically depend on ucontext_t existence.

I believe the approach going forward is supposed to be something more direct, like:

const have_msync = @TypeOf(posix.msync) != void;

But that won't work here (yet?) because posix.zig unilaterally defines an msync.... So you can either fix posix.zig to make that msync void when the underlying system.msync is missing, or just check for @TypeOf(posix.system.msync) != void in debug.zig, and then make sure the default system in posix.zig has a {} msync entry (see #20728 where I fixed a similar issue with freestanding use of debug.zig).

OTOH, the isValidMemory function in debug.zig that your backtrace goes through starts with:

        // We are unable to determine validity of memory for freestanding targets
        if (native_os == .freestanding or native_os == .uefi) return true;

Your native_os is "other", so maybe you can add "other" to this list here. Or, change your target to "freestanding"? (I'm not sure what the difference between "other" and "freestanding" targets is meant to be, so I'm not sure which of those might be the right suggestion.)

Overall, it seems worthwhile to make debug.zig a good example of how to do feature detection "correctly" in Zig, so cleaning it up to have a simple const have_msync = @TypeOf(posix.system.msync) != void (and fixing posix.zig to export msync symbols cleanly) is the right way to go, but I'm definitely not an authoritative source of such opinions.

EDIT: Added missing @TypeOf() on msync checks.

Copy link
Member

@alexrp alexrp Jul 28, 2024

Choose a reason for hiding this comment

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

The intention with the other OS is that you can do the whole "bring your own OS" thing. See #3784. This is distinct from freestanding which is an environment where there explicitly is no OS.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestions here were not quite as straightforward as I made it seem... I've put together a change at #20845. This should solve your specific problem (getting isValidMemory to compile on the other target OS), and I think it does so in the right way, and it includes a test too. But we'll have to wait and see if the Zig core folks think its good...

Copy link
Author

Choose a reason for hiding this comment

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

@rootbeer #20845 fixes my issue and is definitely a higher quality fix than mine. Closing this PR

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