Skip to content

Add missing __LONG_MAX definition in alltypes.h #20752

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 1 commit into from
Nov 20, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 20, 2023

This mirrors an upstream change in musl
(emscripten-core/musl@7cc79d10) and should really have been done as part of #13006.

With had report from a user who was injecting emscripten's sysroot/include path explicitly putting it before the clang's builtin include path which triggered __LONG_MAX to be undefined.

@sbc100 sbc100 requested a review from kripken November 20, 2023 20:40
This mirrors an upstream change in musl
(emscripten-core/musl@7cc79d10) and should
really have been done as part of emscripten-core#13006.

With had report from a user who was injecting emscripten's
`sysroot/include` path explicitly putting it before the clang's builtin
include path which triggered `__LONG_MAX` to be undefined.
@sbc100 sbc100 force-pushed the sysroot_includes_first branch from b9ee397 to c373e5d Compare November 20, 2023 20:58
#else
#define LONG_MAX 0x7fffffffL
#endif
#define LLONG_MAX 0x7fffffffffffffffLL
Copy link
Member

@kripken kripken Nov 20, 2023

Choose a reason for hiding this comment

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

We don't need to define this one in the other header? I tried to find where it was defined elsewhere and saw this:

$ grep -U2 -r LLONG_MAX system/lib/libc/musl/include/
system/lib/libc/musl/include/limits.h-#define LONG_MAX __LONG_MAX
system/lib/libc/musl/include/limits.h-#define ULONG_MAX (2UL*LONG_MAX+1)
system/lib/libc/musl/include/limits.h:#define LLONG_MIN (-LLONG_MAX-1)
system/lib/libc/musl/include/limits.h:#define LLONG_MAX  0x7fffffffffffffffLL
system/lib/libc/musl/include/limits.h:#define ULLONG_MAX (2ULL*LLONG_MAX+1)
system/lib/libc/musl/include/limits.h-
system/lib/libc/musl/include/limits.h-#define MB_LEN_MAX 4

Does that work? It uses LLONG_MAX before it defines it...

Copy link
Member

Choose a reason for hiding this comment

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

otherwise lgtm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to work fine yes. At least this is the same as upstream musl.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like this is just how the C pre-processor works, macros get a expanded at the point of use:

#include <stdio.h>

// BAR is not defined at the point of defining FOO
#define FOO BAR

int main() {
#define BAR 12
  printf("FOO=%d\n", FOO); // prints 12
#define BAR 13
  printf("FOO=%d\n", FOO); // prints 13
}

Copy link
Member

Choose a reason for hiding this comment

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

Oh, really... very interesting. I did not know that about C macros!

@sbc100 sbc100 enabled auto-merge (squash) November 20, 2023 22:22
@sbc100 sbc100 merged commit fdf4456 into emscripten-core:main Nov 20, 2023
@sbc100 sbc100 deleted the sysroot_includes_first branch November 20, 2023 23:00
@@ -4,6 +4,14 @@

#define __BYTE_ORDER __LITTLE_ENDIAN

// Can't use __LONG_MAX__ here since musl's libs does pre-processor comparison
// with 0x7fffffffL directly.

Choose a reason for hiding this comment

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

comparing in the pre-processor should work fine even with #define LONG_MAX __LONG_MAX__? https://godbolt.org/z/zonffPn6o

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, in that case there must be some other reason.. I just assumed that was it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sbc100 added a commit to sbc100/emscripten that referenced this pull request Nov 20, 2023
Followup to emscripten-core#20752.  I must have been running into some other issue.
sbc100 added a commit that referenced this pull request Nov 21, 2023
Followup to #20752.  I must have been running into some other issue.
br0nk pushed a commit to br0nk/emscripten that referenced this pull request Nov 29, 2023
This mirrors an upstream change in musl
(emscripten-core/musl@7cc79d10) and should
really have been done as part of emscripten-core#13006.

With had report from a user who was injecting emscripten's
`sysroot/include` path explicitly putting it before the clang's builtin
include path which triggered `__LONG_MAX` to be undefined.
br0nk pushed a commit to br0nk/emscripten that referenced this pull request Nov 29, 2023
Followup to emscripten-core#20752.  I must have been running into some other issue.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 15, 2023
This is followup to emscripten-core#20752 which was supposed to define __LONG_MAX
rather than LONG_MAX.

You can see the effect of this change on some struct layouts in musl
that contain checks like `#if __LONG_MAX > 0x7fffffff`.  Since
`__LONG_MAX` (and internal-only macro) is now correctly defined we
get some slightly different values/layouts in libc.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 15, 2023
This is followup to emscripten-core#20752 which was supposed to define __LONG_MAX
rather than LONG_MAX.

You can see the effect of this change on some struct layouts in musl
that contain checks like `#if __LONG_MAX > 0x7fffffff`.  Since
`__LONG_MAX` (and internal-only macro) is now correctly defined we
get some slightly different values/layouts in libc.
sbc100 added a commit that referenced this pull request Dec 15, 2023
This is followup to #20752 which was supposed to define __LONG_MAX
rather than LONG_MAX.

You can see the effect of this change on some struct layouts in musl
that contain checks like `#if __LONG_MAX > 0x7fffffff`.  Since
`__LONG_MAX` (and internal-only macro) is now correctly defined we
get some slightly different values/layouts in libc.
ambv pushed a commit to ambv/cpython that referenced this pull request Feb 23, 2025
Starting from Emscripten 3.1.50, there is an issue where LONG_BIT is
calculated to 64 for some reason. This is very strange because LONG_MAX
becomes 2^64-1 when calculating LONG_BIT (in limits.h), but it then
becomes 2^32-1 when it is accessed in other places.

I wasn't able to make a minimal code to reproduce a bug. Probably
something is messed up while importing pyconfig.h, etc.

Related: emscripten-core/emscripten#20752
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