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

translate-c fails on macro with assert(1 && "error message") #14642

Closed
travisstaloch opened this issue Feb 14, 2023 · 10 comments · Fixed by #19610
Closed

translate-c fails on macro with assert(1 && "error message") #14642

travisstaloch opened this issue Feb 14, 2023 · 10 comments · Fixed by #19610
Labels
bug Observed behavior contradicts documented or intended behavior translate-c C to Zig source translation feature (@cImport)
Milestone

Comments

@travisstaloch
Copy link
Contributor

Zig Version

0.11.0-dev.1577+11cc1c16f

Steps to Reproduce and Observed Behavior

Sorry if this is a duplicate issue. Seems like it might be. I didn't find anything in a quick search.

It is common in c to provide an error message like this:

// /tmp/tmp.h
#include <assert.h>

#define FOO assert(1 && "error message")

a minimal @cImport in zig:

// /tmp/tmp.zig
const c = @cImport(@cInclude("tmp.h"));

test {
    _ = c;
}

Yields this error:

$ zig test -lc /tmp/tmp.zig -I/tmp
/home/travis/.cache/zig/o/581a6a5a6944f09e8260e88f89c213da/cimport.zig:616:66: error: cannot compare strings with !=
pub const FOO = assert((@as(c_int, 1) != 0) and ("error message" != 0));
                                                 ~~~~~~~~~~~~~~~~^~~~
/tmp/tmp.zig:1:11: error: C import failed: AnalysisFail
const c = @cImport(@cInclude("tmp.h"));
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
referenced by:
    test_0: /tmp/tmp.zig:4:9
    remaining reference traces hidden; use '-freference-trace' to see all reference traces

Expected Behavior

not sure what should happen. this IS a hacky way to show error messages. i'm only reporting it because its common in c.

@travisstaloch travisstaloch added the bug Observed behavior contradicts documented or intended behavior label Feb 14, 2023
@Vexu Vexu added the translate-c C to Zig source translation feature (@cImport) label Feb 14, 2023
@Vexu Vexu added this to the 0.13.0 milestone Feb 14, 2023
@rohlem
Copy link
Contributor

rohlem commented Feb 14, 2023

not sure what should happen.

IIUC pointers in boolean expressions in C are checked against 0 (= the null pointer constant).
I believe if the emitted Zig code was similarly != null instead of != 0 for pointers (which string literals yield), that would solve the issue.

@travisstaloch
Copy link
Contributor Author

I believe if the emitted Zig code was similarly != null instead of != 0 for pointers (which string literals yield), that would solve the issue.

Its not possible to compare a string literal with null. It would need to be first cast ie @ptrCast([*c]const u8, "error message). Perhaps another type of pointer would be better, not sure.

Of course, this assumes that translate-c can detect when a string literal is being compared using an equality operator. And I don't know that this is true.

@easeaico
Copy link

Does anyone know how to fix this problem?

@Vexu
Copy link
Member

Vexu commented Oct 23, 2023

Yes, something like this should do it:

diff --git a/src/translate_c.zig b/src/translate_c.zig
index ab48ee1f5..c77576cb0 100644
--- a/src/translate_c.zig
+++ b/src/translate_c.zig
@@ -5969,6 +5969,9 @@ fn macroIntToBool(c: *Context, node: Node) !Node {
     if (isBoolRes(node)) {
         return node;
     }
+    if (node.tag() == .string_literal) {
+        return Tag.true_literal.init();
+    }
 
     return Tag.not_equal.create(c.arena, .{ .lhs = node, .rhs = Tag.zero_literal.init() });
 }

@VoilaNeighbor
Copy link

VoilaNeighbor commented Apr 11, 2024

When is this going to be merged? Seems like translate-c is already producing a comparison with pointers but that still does not work with string literals.

A real world example with cglm:

/*!
 * @brief mupliply N mat4 matrices and store result in dest
 *
 * this function lets you multiply multiple (more than two or more...) matrices
 * <br><br>multiplication will be done in loop, this may reduce instructions
 * size but if <b>len</b> is too small then compiler may unroll whole loop,
 * usage:
 * @code
 * mat m1, m2, m3, m4, res;
 *
 * glm_mat4_mulN((mat4 *[]){&m1, &m2, &m3, &m4}, 4, res);
 * @endcode
 *
 * @warning matrices parameter is pointer array not mat4 array!
 *
 * @param[in]  matrices mat4 * array
 * @param[in]  len      matrices count
 * @param[out] dest     result
 */
CGLM_INLINE
void
glm_mat4_mulN(mat4 * __restrict matrices[], uint32_t len, mat4 dest) {
  uint32_t i;

#ifndef NDEBUG
  assert(len > 1 && "there must be least 2 matrices to go!");
#endif

  glm_mat4_mul(*matrices[0], *matrices[1], dest);

  for (i = 2; i < len; i++)
    glm_mat4_mul(dest, *matrices[i], dest);
}

Translated:

pub inline fn glm_mat4_mulN(arg_matrices: [*c][*c]mat4, arg_len: u32, arg_dest: [*c]vec4) void {
    var matrices = arg_matrices;
    _ = &matrices;
    var len = arg_len;
    _ = &len;
    var dest = arg_dest;
    _ = &dest;
    var i: u32 = undefined;
    _ = &i;
    _ = !!((len > @as(u32, @bitCast(@as(c_int, 1)))) and ("there must be least 2 matrices to go!" != null)) or ((blk: {
        _assert("len > 1 && \"there must be least 2 matrices to go!\"", "C:\\Users\\BaiDongJie\\Custom\\Project\\FIG-dual\\lib\\cglm-v0.9.4/mat4.h", @as(c_uint, @bitCast(@as(c_int, 380))));
        break :blk @as(c_int, 0);
    }) != 0);
    glm_mat4_mul(@as([*c]vec4, @ptrCast(@alignCast(&matrices[@as(c_uint, @intCast(@as(c_int, 0)))].*))), @as([*c]vec4, @ptrCast(@alignCast(&matrices[@as(c_uint, @intCast(@as(c_int, 1)))].*))), dest);
    {
        i = 2;
        while (i < len) : (i +%= 1) {
            glm_mat4_mul(dest, @as([*c]vec4, @ptrCast(@alignCast(&matrices[i].*))), dest);
        }
    }
}

@travisstaloch
Copy link
Contributor Author

Yes, something like this should do it:

diff --git a/src/translate_c.zig b/src/translate_c.zig
index ab48ee1f5..c77576cb0 100644
--- a/src/translate_c.zig
+++ b/src/translate_c.zig
@@ -5969,6 +5969,9 @@ fn macroIntToBool(c: *Context, node: Node) !Node {
     if (isBoolRes(node)) {
         return node;
     }
+    if (node.tag() == .string_literal) {
+        return Tag.true_literal.init();
+    }
 
     return Tag.not_equal.create(c.arena, .{ .lhs = node, .rhs = Tag.zero_literal.init() });
 }

@Vexu I applied your patch and it does indeed work, producing the following

...
pub const FOO = assert((@as(c_int, 0) != 0) and true);

when given

#include <assert.h>

#define FOO assert(0 && "error message")

As you can see, "error message" gets lost. Do you think it would be better to preserve the string literal, maybe translating it as the following instead of true?

@intFromPtr("error message") != 0

@travisstaloch
Copy link
Contributor Author

travisstaloch commented Apr 11, 2024

as mentioned by @rohlem above, I noticed that the following produces invalid zig code, comparing a str literal with null

// /tmp/tmp.h
void foo() { if(0 && "error message") {} }
$ zig translate-c /tmp/tmp.h

result:

pub export fn foo() void {
    if (false and ("error message" != null)) {}
}

Should this be supported? If so, any idea how to fix it? I searched around in src/translate_c.zig a bit but not sure how this might be done.

@Vexu
Copy link
Member

Vexu commented Apr 11, 2024

Try here:

zig/src/translate_c.zig

Lines 2088 to 2091 in f7a76bd

.Pointer => {
// node != null
return Tag.not_equal.create(c.arena, .{ .lhs = node, .rhs = Tag.null_literal.init() });
},

@travisstaloch
Copy link
Contributor Author

thanks @Vexu will work on this tomorrow. will make a separate pr unless you think i should just add it #19610.

Vexu pushed a commit that referenced this issue Apr 11, 2024
travisstaloch added a commit to travisstaloch/zig that referenced this issue Apr 11, 2024
@travisstaloch
Copy link
Contributor Author

Try here:

zig/src/translate_c.zig

Lines 2088 to 2091 in f7a76bd

.Pointer => {
// node != null
return Tag.not_equal.create(c.arena, .{ .lhs = node, .rhs = Tag.null_literal.init() });
},

just submitted #19625 with a fix at this location

Vexu pushed a commit that referenced this issue Apr 12, 2024
this is a follow up to #19610 with fix suggested by Vexu in
#14642 (comment)
@andrewrk andrewrk modified the milestones: 0.15.0, 0.12.0 Apr 12, 2024
TUSF pushed a commit to TUSF/zig that referenced this issue May 9, 2024
TUSF pushed a commit to TUSF/zig that referenced this issue May 9, 2024
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 translate-c C to Zig source translation feature (@cImport)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants