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

NULL isn't coerced to optional function type in translated c header files #5596

Open
shtanton opened this issue Jun 12, 2020 · 13 comments
Open
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. translate-c C to Zig source translation feature (@cImport)
Milestone

Comments

@shtanton
Copy link
Contributor

The problem might be more general than this, but when importing gtk and using g_signal_connect I get the error:

Semantic Analysis [343/726] ./zig-cache/o/9PlMMzdvgCrNH_iYsNdGkXRhWGclysI8W9p8Q8smhCA-WIRXI80a2BFzjsxrgBg-/cimport.zig:47023:170: error: expected type '?fn(?*c_void, ?*.cimport:2:11.struct__GClosure) callconv(.C) void', found '?*c_void'
pub inline fn g_signal_connect(instance: var, detailed_signal: var, c_handler: var, data: var) @TypeOf(g_signal_connect_data(instance, detailed_signal, c_handler, data, NULL, @intToPtr(GConnectFlags, 0))) {

With the issue seeming to be that it can't coerce NULL to the optional function because it's already given NULL the type of ?*c_void

If this is a bug that needs a not crazy complicated fix, I can try making a pull request for it.

@Vexu
Copy link
Member

Vexu commented Jun 12, 2020

This is quite tricky to fix since g_signal_connect is a macro and we have no type info available. Fixing this in translate-c would require pretty much re-implementing c type system in comptime checks.

One reasonable solution would be to allow casts from ?*c_void to other optional pointer types when comptime known to be null.

@Vexu Vexu added the bug Observed behavior contradicts documented or intended behavior label Jun 12, 2020
@Vexu Vexu added this to the 0.7.0 milestone Jun 12, 2020
@shtanton
Copy link
Contributor Author

Is it worth me having a go at fixing this as a first contribution or should I find something simpler first?

@Vexu
Copy link
Member

Vexu commented Jun 12, 2020

I definitely recommend something simpler. Issues labeled contributor friendly are good choices.

@Vexu
Copy link
Member

Vexu commented Jun 12, 2020

I think we could also get away with just translating NULL as null. @shtanton if you want to try fixing this parseCPrimaryExpr in src-self-hosted/translate_c.zig is the place to look.

@Vexu Vexu added contributor friendly This issue is limited in scope and/or knowledge of Zig internals. translate-c C to Zig source translation feature (@cImport) labels Jun 12, 2020
@shtanton
Copy link
Contributor Author

So I've spent some time digging around the codebase and testing various stuff. Interestingly the error I get compiling with the master branch is different to with 0.6.0, although I think it still seems straightforward to fix. I've also found a pretty minimal example of the error:
test.zig:

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

pub fn main() !void {
    std.debug.warn("{}", .{c.donothing});
}

macro.h:

#define NULL ((void*)0)
#define donothing dosomething(NULL)

int dosomething(int *a);

macro.c:

#define NULL ((void*)0)

int dosomething(int *a) {
    if (a == NULL) {
        return 5;
    } else {
        return *a;
    }
}

Then compile with ${BUILDDIR}/zig build-exe test.zig --c-source macro.c -lc -I. which produces the compile error:

zig-cache/o/82bPbbbBl_k67Gdy8kC8-sdaDUFPtqHUrldBh-vP6DTYiY29lVrGPPoUz4Qdi5SE/cimport.zig:360:98: error: integer value 0 cannot be coerced to type '*c_void'
pub const NULL = (if (@typeInfo(?*c_void) == .Pointer) @intToPtr(?*c_void, 0) else @as(?*c_void, 0));
                                                                                                 ^
zig-cache/o/82bPbbbBl_k67Gdy8kC8-sdaDUFPtqHUrldBh-vP6DTYiY29lVrGPPoUz4Qdi5SE/cimport.zig:218:35: note: referenced here
pub const donothing = dosomething(NULL);
                                  ^
./test.zig:7:29: note: referenced here
    std.debug.warn("{}", .{c.donothing});

I think the way to fix this is to check if the type is a pointer or an optional pointer instead of only checking if it is a pointer, does that sound ok?

@Vexu
Copy link
Member

Vexu commented Jun 14, 2020

That's a recent regression from when we started translating *void as ?*c_void.
I think a good fix would be to add a some kind of intToPtr-able function to std.meta and calling that instead of doing @typeInfo(?*c_void) == .Pointer.

Thinking about it now, we could simplify translate-c by adding a fn cast(comptime T: type, val: var) T to std.c and calling that instead of building these complicated ifs.

@shtanton
Copy link
Contributor Author

shtanton commented Jun 15, 2020

I can add a function canIntToPtr to std.meta and then use it in the condition. I'll leave that cast function to someone more competent than me

@Vexu
Copy link
Member

Vexu commented Jun 15, 2020

Implementing the cast function should ve easy enough, just copy the if statement currently being generated and make translate-c call it. You can find an example of translate-c calling a std function in qualTypeToLog2IntRef.

@shtanton
Copy link
Contributor Author

I implemented std.meta.isPointer and added some tests and modified translate-c.zig to use it which it does correctly. Now my branch is giving the same error as I was originally getting, it's able to assign a type to NULL but the type it assigns is ?*c_void which can't be cast to another optional unless the child types can be cast.

I thought this could be fixed by checking if the int being cast is comptime 0 and target type is ?*c_void and then using the type of null if it is and ?*c_void if not. I came up with something like this:

comptime const NULL = (if (@import("std").meta.isPointer(dest)) (if (val == 0 and dest == ?*c_void) null else @intToPtr(dest, val)) else @as(dest, val));

which works only when it's local scoped and I'm not sure why. It needs comptime to work which isn't available for globals (I think because global constants are already comptime?). When it's global it can't infer a type.

The only other option I can think of is to check the c AST specifically for (void*)0 and translate it to null but it would miss some weird cases where the 0 is known at compile time but isn't a literal.

@shtanton
Copy link
Contributor Author

shtanton commented Jun 18, 2020

I've done a bit more experimenting, for some reason

comptime const NULL = null;

is fine if it's scoped but

const NULL = null;

as a global variable errors with variable of type '(null)' not allowed

Why does it make a difference whether the variable is global, I'd understand if it always worked or didn't, but a local variable can be null while a global can't?

Maybe another option is to allow coercion from ?*c_void to ?*T if it is known at compile time to be a null pointer? Edit: just realised you suggested this at the start so I'll explore it some more

@shtanton
Copy link
Contributor Author

I've added type coercion from ?*c_void to any optional if it is comptime 0.

I've gone back to the cast function and I think it needs to be more complicated. @intToEnum, @enumToInt and @ptrToInt at least need to be included as @as differs from a cast in c here as far as I can tell. I don't know how I'd figure out the exhaustive list of cases for casts, I could just look through all the casting builtins and check they're included if they need to be.

@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Aug 13, 2020
@Vexu Vexu modified the milestones: 0.8.0, 0.9.0 Jun 4, 2021
@andrewrk
Copy link
Member

Can we get some example C code added to this issue that the test case wanted to be translated?

@andrewrk andrewrk modified the milestones: 0.9.0, 0.11.0 Nov 24, 2021
@jcmoyer
Copy link
Contributor

jcmoyer commented Nov 25, 2021

This also happens with Lua's lua_pcall.

Reduced test case:

#include <stddef.h>
typedef void (*function)();
extern void foo(function);
#define bar() foo(NULL)

After zig translate-c:

pub const NULL = @import("std").zig.c_translation.cast(?*c_void, @as(c_int, 0));
pub const function = ?fn (...) callconv(.C) void;
pub extern fn foo(function) void;
pub inline fn bar() @TypeOf(foo(NULL)) {
    return foo(NULL);
}

Error:

testcase.zig:6:33: error: expected type '?fn(...) callconv(.C) void', found '?*c_void'
pub inline fn bar() @TypeOf(foo(NULL)) {
                                ^
testcase.zig:6:33: note: optional type child '*c_void' cannot cast into optional type child 'fn(...) callconv(.C) void'
pub inline fn bar() @TypeOf(foo(NULL)) {

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 contributor friendly This issue is limited in scope and/or knowledge of Zig internals. translate-c C to Zig source translation feature (@cImport)
Projects
None yet
Development

No branches or pull requests

4 participants