-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Revise alltypes.h
update process. NFC
#24773
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
base: main
Are you sure you want to change the base?
Conversation
Align it with musl, i.e. only update `alltypes.h.in` and regenerate `alltypes.h` using a script.
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.
Great!
|
||
musl_srcdir="$PWD/../../.." | ||
musl_includedir="$musl_srcdir/include" | ||
sed -f $musl_srcdir/tools/mkalltypes.sed ./alltypes.h.in $musl_includedir/alltypes.h.in > alltypes.h |
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.
Can you put this up in system/lib
with the other scripts we have? (or maybe in system/lib/libc/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.
|
||
#if defined(__FLT_EVAL_METHOD__) && __FLT_EVAL_METHOD__ == 0 |
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.
Do we need this if/else block here? I don't see it many of the upstream .in files so I think maybe we don't need 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.
It appears to be defined only for 32-bit targets in musl. But you're right, this conditional statement is probably unnecessary for Wasm. Commit 4a3bcba resolves this.
|
||
#ifndef __cplusplus | ||
TYPEDEF unsigned wchar_t; | ||
#ifdef __WCHAR_TYPE__ |
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 can just assume WCHAR_TYPE is present I think.
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.
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.
Ah, it seems the __cplusplus
exclusion was still needed. ecefbbd
#endif | ||
TYPEDEF unsigned wint_t; | ||
#endif | ||
TYPEDEF __WINT_TYPE__ wint_t; |
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.
This is not needed I think.
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.
This was previously defined here:
emscripten/system/lib/libc/musl/arch/emscripten/bits/alltypes.h
Lines 52 to 55 in 90250aa
#if defined(__NEED_wint_t) && !defined(__DEFINED_wint_t) | |
typedef __WINT_TYPE__ wint_t; | |
#define __DEFINED_wint_t | |
#endif |
WIntType
in LLVM seems to default to SignedInt
, see e.g.:
https://github.com/llvm/llvm-project/blob/llvmorg-20.1.8/clang/lib/Basic/TargetInfo.cpp#L131
So, this ensures it remains signed, otherwise, it defaults to unsigned.
emscripten/system/lib/libc/musl/arch/emscripten/bits/alltypes.h
Lines 278 to 281 in 4bb13ba
#if defined(__NEED_wint_t) && !defined(__DEFINED_wint_t) | |
typedef unsigned wint_t; | |
#define __DEFINED_wint_t | |
#endif |
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.
(commit 4554600 might also be relevant here)
#define __DEFINED_blksize_t | ||
#endif | ||
|
||
#if defined(__NEED_blkcnt_t) && !defined(__DEFINED_blkcnt_t) | ||
typedef int blkcnt_t; | ||
typedef _Int64 blkcnt_t; |
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.
Some of these look like real ABI changes. Perhaps we should try to make this changes actually NFC and do any ABI changes as followups? (or land them first).
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.
This is a bit tricky to review, but blkcnt_t
should still be 32-bit, see:
emscripten/system/lib/libc/musl/arch/emscripten/bits/alltypes.h
Lines 25 to 29 in 82662c0
// XXX EMSCRIPTEN: ensure it's always 32-bits even in wasm64 | |
#if defined(__NEED_blkcnt_t) && !defined(__DEFINED_blkcnt_t) | |
typedef int blkcnt_t; | |
#define __DEFINED_blkcnt_t | |
#endif |
These 32-bit type overrides originate from alltypes.h.in
:
emscripten/system/lib/libc/musl/arch/emscripten/bits/alltypes.h.in
Lines 17 to 23 in 82662c0
// XXX EMSCRIPTEN: ensure it's always 32-bits even in wasm64 | |
TYPEDEF int blkcnt_t; | |
TYPEDEF int blksize_t; | |
TYPEDEF int clock_t; | |
TYPEDEF unsigned int dev_t; | |
TYPEDEF int suseconds_t; | |
TYPEDEF unsigned int wctype_t; |
(and
alltypes.h
is generated from this file)
The musl default definitions starts at line 95:
#define __LITTLE_ENDIAN 1234 |
Also, other.test_gen_struct_info
still passes on this branch, any ABI break would likely have caused it to fail.
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.
With commit 6809780 and the following local change:
--- a/tools/system_libs.py
+++ b/tools/system_libs.py
@@ -62,7 +62,7 @@ def glob_in_path(path, glob_pattern, excludes=()):
def get_base_cflags(build_dir, force_object_files=False, preprocess=True):
# Always build system libraries with debug information. Non-debug builds
# will ignore this at link time because we link with `-strip-debug`.
- flags = ['-g', '-sSTRICT', '-Werror']
+ flags = ['-sSTRICT', '-Werror']
if settings.LTO and not force_object_files:
flags += ['-flto=' + settings.LTO]
if settings.RELOCATABLE:
I now see:
$ ./embuilder.py build sysroot libc{,-mt} --force
$ ./embuilder.py build sysroot libc{,-mt} --force --wasm64
$ sha256sum cache/sysroot/lib/wasm{32,64}-emscripten/libc{,-mt}.a
dd54801051079bd19040b4d826ded15f7352be10af1ad70d7aa74ce3befb6239 cache/sysroot/lib/wasm32-emscripten/libc.a
6fafd19332c11860b2365efec265fc037080ec97d1a84fafcca9ff338461bd70 cache/sysroot/lib/wasm32-emscripten/libc-mt.a
0310ba126aa4ca060538b6203105b71bfb115c24bcf82d6412c50e0fe041b74c cache/sysroot/lib/wasm64-emscripten/libc.a
66ff6308831fb3df57cc884c10207f73135f4399920e871e83a85f9ee1ac9ab0 cache/sysroot/lib/wasm64-emscripten/libc-mt.a
$ git checkout revise-musl-alltypes-h
$ ./embuilder.py build sysroot libc{,-mt} --force
$ ./embuilder.py build sysroot libc{,-mt} --force --wasm64
$ sha256sum cache/sysroot/lib/wasm{32,64}-emscripten/libc{,-mt}.a
dd54801051079bd19040b4d826ded15f7352be10af1ad70d7aa74ce3befb6239 cache/sysroot/lib/wasm32-emscripten/libc.a
6fafd19332c11860b2365efec265fc037080ec97d1a84fafcca9ff338461bd70 cache/sysroot/lib/wasm32-emscripten/libc-mt.a
0310ba126aa4ca060538b6203105b71bfb115c24bcf82d6412c50e0fe041b74c cache/sysroot/lib/wasm64-emscripten/libc.a
66ff6308831fb3df57cc884c10207f73135f4399920e871e83a85f9ee1ac9ab0 cache/sysroot/lib/wasm64-emscripten/libc-mt.a
So, when built without debug information, the libc static libraries are byte-for-byte identical to those built from the main branch.
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.
This is a bit tricky to review, but
blkcnt_t
should still be 32-bit, see:emscripten/system/lib/libc/musl/arch/emscripten/bits/alltypes.h
Lines 25 to 29 in 82662c0
// XXX EMSCRIPTEN: ensure it's always 32-bits even in wasm64 #if defined(__NEED_blkcnt_t) && !defined(__DEFINED_blkcnt_t) typedef int blkcnt_t; #define __DEFINED_blkcnt_t #endif These 32-bit type overrides originate from
alltypes.h.in
:emscripten/system/lib/libc/musl/arch/emscripten/bits/alltypes.h.in
Lines 17 to 23 in 82662c0
// XXX EMSCRIPTEN: ensure it's always 32-bits even in wasm64 TYPEDEF int blkcnt_t; TYPEDEF int blksize_t; TYPEDEF int clock_t; TYPEDEF unsigned int dev_t; TYPEDEF int suseconds_t; TYPEDEF unsigned int wctype_t; (and
alltypes.h
is generated from this file)
The musl default definitions starts at line 95:
#define __LITTLE_ENDIAN 1234 Also,
other.test_gen_struct_info
still passes on this branch, any ABI break would likely have caused it to fail.
Oh that is rather annoying/strange. It makes things like git grep typedef.*blkcnt_t
a little annoying.
Can you add a comment to the end of the emscripen-specific file saying something like this:
// END EMSCRIPTEN-SPECIFIC DEFINITIONS
//
// Below here are the shared musl definitions. The emscripten-specific definitions above will take precedence
// due to the `__DEFINED_` macro system.
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.
Good idea! I added a comment via 4eba06a. We could perhaps consider marking this file with linguist-generated=true
in .gitattributes
to ensure the diff is hidden by default, though I don't feel strongly about 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.
Nah, I don't think this file should change often, and when it does I really want to see the details since this is ABI-defining stuff.
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.
Thanks for working on this! LGTM with one final comment.
Align it with musl, i.e. only update
alltypes.h.in
and regeneratealltypes.h
using a script.