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

Runtime behavior changes depending on read-only variable access #4947

Closed
koljakube opened this issue Apr 5, 2020 · 3 comments · Fixed by #4970
Closed

Runtime behavior changes depending on read-only variable access #4947

koljakube opened this issue Apr 5, 2020 · 3 comments · Fixed by #4970
Labels
bug Observed behavior contradicts documented or intended behavior stage1 The process of building from source via WebAssembly and the C backend.
Milestone

Comments

@koljakube
Copy link

While playing around with wrapping ncurses, I ran into the following problem:

Given the following files:

main.zig:

const std = @import("std");
const nc = @import("import.zig");
const c = nc.c;

pub fn main() !void {
    _ = c.initscr();
    nc.start_color();

    // const pCLRS = &c.COLORS;

    if (nc.COLORS) |clrs| {
        std.debug.warn("COLORS: {}\n", .{clrs});
    }

    std.time.sleep(1000000000);
}

import.zig:

const std = @import("std");
pub const c = @cImport({
    @cInclude("ncurses.h");
});

pub var COLORS: ?i32 = null;

pub fn start_color() void {
    _ = c.start_color();

    COLORS = c.COLORS;
}

Compile and run with: zig build-exe --library ncurses main.zig && ./main

The output is "COLORS: 0". By commenting in line 9 of main.zig, the output changes to "COLORS: 256". Tested on macOS 10.15.4 using zig 0.5.0+1568470c4, verified on Linux.

I managed to create this problem without pointers (with a warn() instead of the pure read), but build this example first. My suspicion is that the access triggers some kind of runtime behavior, and that explicitly marking the COLORS assignment in start_color() as a runtime operation would fix this, but I can't prove it yet.

A runtime equivalent to comptime might be a good idea anyhow.

@LemonBoy
Copy link
Contributor

LemonBoy commented Apr 6, 2020

Global variables suck.

Ncurses defines an external variable named COLORS and so does import.zig. The compiler is dumb and clumps together all the (scope-)global variables together without any kind of namespacing, this means that any variable named COLORS will alias the other with the same name.

Adding const pCLRS = &c.COLORS; generates a nice conflict that translates into the following LLVM IR:

@COLORS = external global i32, align 4
@COLORS.1 = internal unnamed_addr global %"?i32" { i32 undef, i1 false }, align 4

You were lucky enough, the extern variable won over the global one and the code behaves as intended.

@andrewrk andrewrk added the bug Observed behavior contradicts documented or intended behavior label Apr 6, 2020
@andrewrk andrewrk added this to the 0.7.0 milestone Apr 6, 2020
@andrewrk
Copy link
Member

andrewrk commented Apr 6, 2020

To be clear the intended behavior here is that pub vars cannot clobber external symbols. The pub var COLORS is the one that is supposed to get the mangling/namespacing.

@andrewrk andrewrk added the stage1 The process of building from source via WebAssembly and the C backend. label Apr 6, 2020
LemonBoy added a commit to LemonBoy/zig that referenced this issue Apr 7, 2020
Extend the logic used for function definitions to variables.

Closes ziglang#4947
@koljakube
Copy link
Author

Thanks for the explanation, helps a lot to understand what's going on. That lets me work around the problem for now.

andrewrk pushed a commit that referenced this issue Apr 7, 2020
Extend the logic used for function definitions to variables.

Closes #4947
@andrewrk andrewrk modified the milestones: 0.7.0, 0.6.0 Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior 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