Skip to content

Change definition of true and false macros to be js friendly #24255

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

Merged
merged 6 commits into from
May 6, 2025

Conversation

hoodmane
Copy link
Collaborator

@hoodmane hoodmane commented May 5, 2025

No description provided.

@hoodmane
Copy link
Collaborator Author

hoodmane commented May 5, 2025

@sbc100 I added a test but curiously it does not pass because stdbool.h is included from upstream/lib/clang/21/include/stdbool.h and not from cache/sysroot/include/stdbool.h.

@sbc100
Copy link
Collaborator

sbc100 commented May 5, 2025

@sbc100 I added a test but curiously it does not pass because stdbool.h is included from upstream/lib/clang/21/include/stdbool.h and not from cache/sysroot/include/stdbool.h.

But you've seen this fail locally for you?

@hoodmane
Copy link
Collaborator Author

hoodmane commented May 5, 2025

Yes, I've been using this in Pyodide for years:
https://github.com/pyodide/pyodide/blob/main/src/core/types.h#L12-L17

I just had to copy paste it into another project and figured it would be nice to upstream.

@sbc100
Copy link
Collaborator

sbc100 commented May 5, 2025

Yes, I've been using this in Pyodide for years: https://github.com/pyodide/pyodide/blob/main/src/core/types.h#L12-L17

I just had to copy paste it into another project and figured it would be nice to upstream.

So when you see it fail I assume that failure is also coming from the clang version this header? If so, I guess this fix needs to happen upstream in clang instead?

@sbc100
Copy link
Collaborator

sbc100 commented May 5, 2025

Another solution would be to using -std=c17 (or C++) in which case these macros won't get defined.

@hoodmane
Copy link
Collaborator Author

hoodmane commented May 5, 2025

Yeah, I guess the system search path looks like:

.../emsdk-dev/upstream/lib/clang/21/include
.../emscripten/cache/sysroot/include

so the clang include files are preferred over the emscripten files.

@sbc100
Copy link
Collaborator

sbc100 commented May 5, 2025

We could potentially inject a fix like your into our em_asm/em_js headers? I would certainly want to guard the fix behind if defined(true) && defined(false) though so it was a no-op in C++ / C-17

@hoodmane hoodmane force-pushed the js-friendly-true-and-false branch from 6eadd5c to f7cdabe Compare May 5, 2025 22:10
@hoodmane
Copy link
Collaborator Author

hoodmane commented May 5, 2025

Makes sense to me, I did that.

@hoodmane
Copy link
Collaborator Author

hoodmane commented May 6, 2025

Okay I added the comments you suggested.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I wonder if its worth adding this without also adding something like EM_JS_MACROS.

I would imagine the best place for something like this would be alongside something like EM_JS_MACROS.

Is the idea that there could be other users out there who try to write EM_JS_MACROS and this will help them avoid the pain you went through trying to debug stuff?

@hoodmane
Copy link
Collaborator Author

hoodmane commented May 6, 2025

Yeah. A few months ago, someone was asking how to make EM_JS_MACROS in an issue and I told them that this was the one tricky thing. I think it would make sense to add an EM_JS_MACROS macro at the same time. It is super useful if you have to do struct accesses:

struct MyStruct {
   void* field1;
   void* field2;
}

#define MyStruct_field1_offset 0
#define MyStruct_field2_offset 4

_Static_assert(offsetof(MyStruct, field1) == MyStruct_field1_offset);
_Static_assert(offsetof(MyStruct, field2) == MyStruct_field2_offset);

#define MyStruct_field1(x) HEAPU32[x/4 + MyStruct_field1_offset]
#define MyStruct_field2(x) HEAPU32[x/4 + MyStruct_field2_offset]

EM_JS_MACROS(void, f, (MyStruct* arg), {
   const field1 = MyStruct_field1(x);
   const field2 = MyStruct_field2(x);
});

Another common use case is for sharing bitflags between C and JS:

#define FLAG1 (1 << 0)
#define FLAG2 (1 << 1)
...

EMSCRIPTEN_KEEPALIVE int get_flags() {
    return FLAG1 | FLAG2;
}

EM_JS_MACROS(void, process_flags, (int flags), {
   if (flags & FLAG1) {
     // handle FLAG1
   }
   if (flags & FLAG2) {
     // handle FLAG2
   }
});

But the biggest reason for me is to inject standard error handling everywhere so that it's easier to implement a C calling convention in JS:

#define EM_JS_REF(ret, func_name, args, body...)                               \
  EM_JS_MACROS(ret WARN_UNUSED, func_name, args, {                             \
    try    /* intentionally no braces, body already has them */                \
      body /* <== body of func */                                              \
    catch (e) {                                                                \
        LOG_EM_JS_ERROR(func_name, e);                                         \
        Module.handle_js_error(e);                                             \
        return 0;                                                              \
    }                                                                          \
    errNoRet();                                                                \
  })

@hoodmane hoodmane merged commit fb2ea4d into emscripten-core:main May 6, 2025
28 checks passed
@hoodmane hoodmane deleted the js-friendly-true-and-false branch May 6, 2025 22:36
@hoodmane
Copy link
Collaborator Author

hoodmane commented May 6, 2025

Opened #24272.

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.

2 participants