-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
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.
b9ee397
to
c373e5d
Compare
#else | ||
#define LONG_MAX 0x7fffffffL | ||
#endif | ||
#define LLONG_MAX 0x7fffffffffffffffLL |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise lgtm
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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!
@@ -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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followup to emscripten-core#20752. I must have been running into some other issue.
Followup to #20752. I must have been running into some other issue.
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.
Followup to emscripten-core#20752. I must have been running into some other issue.
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.
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.
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.
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
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.