Update core wasm32 typedefs to be compatible with reference-sysroot. by sunfishcode · Pull Request #7799 · emscripten-core/emscripten

sunfishcode · 2019-01-03T00:54:52Z

Change time_t from long (32-bit) to long long (64-bit). This fixes the 2038 bug. It's also consistent with the x32 ABI.

Change the fallback for wchar_t and wint_t from unsigned to int. This is not an ABI change in practice because the headers prefer to use __WCHAR_TYPE__ and __WINT_TYPE__ when they're defined, and clang defines those to be int. So this just makes the fallback cases for other hypothetical compilers consistent with clang. (And fwiw, no one specifically chose int for these, that's just the default in clang if the target doesn't override it. So if anyone feels strongly that wchar_t/wint_t should be something different, we can change them).

Change suseconds_t from long to long long. This matches x32, uses what would otherwise be padding a struct timeval, and generally makes it easier for users to avoid overflow in time calculations.

Change _Reg from __PTRDIFF_TYPE__ (32-bit) to __INT64_TYPE__ (64-bit). _Reg is only used for nlink_t and register_t. For nlink_t, this matches x32, and theoretically supports filesystems that support billions of links. For register_t, since wasm supports native i64 values, a 64-bit int is the closest mapping of this concept.

This PR doesn't currently include any provision for ABI versioning. At least in theory, the wasm32 ABI shouldn't be considered stable yet; the upstream LLVM component hasn't emerged from Experimental status yet (though it is expected to in a few months), though I'm open to practical concerns here.

Change `time_t` from `long` (32-bit) to `long long` (64-bit). This fixes the [2038 bug](https://en.wikipedia.org/wiki/Year_2038_problem). It's also consistent with the [x32 ABI](https://en.wikipedia.org/wiki/X32_ABI). Change the fallback for `wchar_t` and `wint_t` from `unsigned` to `int`. This is not an ABI change in practice because the headers prefer to use `__WCHAR_TYPE__` and `__WINT_TYPE__` when they're defined, and clang defines those to be `int`. So this just makes the fallback cases for other hypothetical compilers consistent with clang. (And fwiw, no one specifically chose `int` for these, that's just the default in clang if the target doesn't override it. So if anyone feels strongly that wchar_t/wint_t should be something different, we can change them). Change `suseconds_t` from `long` to `long long`. This matches x32, uses what would otherwise be padding a `struct timeval`, and generally makes it easier for users to avoid overflow in time calculations. Change `_Reg` from `__PTRDIFF_TYPE__` (32-bit) to `__INT64_TYPE__` (64-bit). `_Reg` is only used for `nlink_t` and `register_t`. For `nlink_t`, this matches x32, and theoretically supports filesystems that support billions of links. For `register_t`, since wasm supports native `i64` values, a 64-bit int is the closest mapping of this concept.

kripken · 2019-01-03T02:05:38Z

This seems good to me. Let's see what others think though. Also would be good to confirm that this is in the right direction for #7649 (cc @rianhunter).

I suspect some tests will fail here, in particular library_syscall.js may depend on those types, and if so would need updates.

This will require a minor version bump when landing so system libraries are rebuilt.

This includes off_t, ino_t, and several other POSIX types.

sunfishcode · 2019-01-03T02:12:42Z

It turns out that I was confused about the relationship between alltypes.h.in and alltypes.h. I've updated a few more types now so that people can see what I'm proposing, though I still need to sanity-check everything.

rianhunter · 2019-01-03T02:17:50Z

These changes are consistent with changes I also had plans to make in #7649. My only request is the addition of a minimal "emscripten metadata" section for the generated wasm before making ABI changes like these. Without that it will be impossible for wasmjit (and others) to detect that this code is incompatible with the ABI it currently targets. Users who unwittingly build wasm files with incompatible versions of emscripten won't be able to easily discover the root problem, and think either wasmjit or emscripten is broken.

Update library_syscall.js to reflect that several types are i64 now. Struct stat is changing, so let's also update it to something with fewer odd legacy fields.

rianhunter · 2019-01-03T07:41:28Z

system/lib/libc/musl/arch/emscripten/bits/stat.h

-/* copied from kernel definition, but with padding replaced
- * by the corresponding correctly-sized userspace types. */
-
 struct stat


what struct stat definition is this mirroring? is it the stat used in the x32 linux ABI?

Yes, it's the x32 linux ABI. I've now added a comment.

sunfishcode · 2019-01-03T17:47:24Z

These changes are consistent with changes I also had plans to make in #7649. My only request is the addition of a minimal "emscripten metadata" section for the generated wasm before making ABI changes like these. Without that it will be impossible for wasmjit (and others) to detect that this code is incompatible with the ABI it currently targets. Users who unwittingly build wasm files with incompatible versions of emscripten won't be able to easily discover the root problem, and think either wasmjit or emscripten is broken.

Would you be willing to submit a patch for this part? I don't know what you need here, and I'm not very familiar with the parts of Emscripten and Binaryen that would be involved.

rianhunter · 2019-01-03T20:29:58Z

That is fair, I can do that ASAP.

kripken · 2019-01-03T20:51:37Z

src/library.js


-  difftime: function(time1, time0) {
-    return time1 - time0;
+  difftime: function(time1lo, time1hi, time0lo, time0hi) {


please use l, h instead of lo, hi for consistency with other similar things in this file

kripken · 2019-01-03T20:53:25Z

src/library_syscall.js

+      return low;
+    },
+    // Like get64, but read high first and low second.
+    get64hilo: function() {


how about reversed or highLow instead of hilo?

Fixed; changed to reversed.

kripken · 2019-01-03T20:54:47Z

src/library_syscall.js

+    // Like get64, but read high first and low second.
+    get64hilo: function() {
+      var high = SYSCALLS.get(), low = SYSCALLS.get();
+      if (low >= 0) assert(high === 0);


could this check with a >> 31 like in library.js? or is there a reason to do it differently here?

This was just copied from the existing get64. I've now updated both versions to use the >> 31 check.

kripken · 2019-01-03T20:57:04Z

system/lib/libc/musl/ldso/dynlink.c

 	int prot = PROT_READ|PROT_WRITE, flags = MAP_ANONYMOUS|MAP_PRIVATE;
 #ifdef SYS_mmap2
-	p = (void *)__syscall(SYS_mmap2, 0, n, prot, flags, -1, 0);
+	p = (void *)__syscall(SYS_mmap2, 0, n, prot, flags, -1, 0, 0);


this change is in general musl code, not in the emscripten-specific part - why is it needed?

Ah, you're right. It isn't needed. I've now reverted this change.

kripken · 2019-01-03T20:57:36Z

system/lib/libc/musl/src/mman/mmap.c

+        off_t pgoffset = off / UNIT;
+	return (void *)syscall(SYS_mmap2, start, len, prot, flags, fd,
+                               (int32_t)pgoffset, ((int32_t)(pgoffset >> 32)));
 #else


same question as above

Same answer :-).

kripken · 2019-01-03T20:58:45Z

tests/test_other.py

+        (['-O2'], 18, ['assert'], ['waka'], 12616,  17,   15, 34), # noqa
+        (['-O3'],  7, [],         [],        2706,  10,    2, 22), # noqa; in -O3, -Os and -Oz we metadce
+        (['-Os'],  7, [],         [],        2706,  10,    2, 22), # noqa
+        (['-Oz'],  7, [],         [],        2706,  10,    2, 22), # noqa


looks like some extra function survives metadce, so it's actually used - what is it?

It's nullFunc_jiji. We have one more unique signature now.

I'm confused, I don't see any nullFunc methods when I build say ./emcc tests/hello_world.c -Os -profiling. Reading emscripten.py those should only be emitted when ASSERTIONS are on. So that doesn't explain the change in the metadce levels here, -O3, -Os, -Oz?

kripken · 2019-01-03T20:59:10Z

tests/zlib/ref.txt

@@ -1,4 +1,4 @@
-zlib version 1.2.5 = 4688, compile flags = 85
+zlib version 1.2.5 = 4688, compile flags = 149


what's this?

zlib apparently prints out some flags and one of them reflects the size of off_t.

https://github.com/kripken/emscripten/blob/incoming/tests/zlib/zutil.c#L55
https://github.com/kripken/emscripten/blob/incoming/tests/zlib/example.c#L531

juj · 2019-01-04T10:36:15Z

emscripten-version.txt

@@ -1 +1 @@
-"1.38.21"
+"1.38.22"


Instead of bumping minor version, we should bump to 1.39.0, because this is a breaking ABI change. (like we did in #5916 (comment))

I'm not opposed to that, but I don't think we have a strict rule here - even minor version number updates can break ABI, e.g. with an LLVM update. I did agree in the linked issue that we should do a major one because it was a large change, and because it could be particularly confusing and error-prone, but in this PR here the change is minor (unless I'm missing something), so I don't feel strongly either way.

+1, I think this is worth bumping to 1.39
I don't recall whether we came to a strict rule but I hope we can eventually (soon) get to the point where we can have a strict rule of bumping the minor for ABI breaks.

Sounds good. Then we should land this with a minor version, wait to see we are reasonably stable, then bump the major version, as documented here: https://github.com/kripken/emscripten/blob/incoming/docs/process.md#major-version-update-1xy-to-1x10

rianhunter · 2019-01-05T00:46:06Z

ABI versioning PR here: #7815

rianhunter · 2019-02-11T21:52:22Z

Hey @sunfishcode could you use some help with this? I have some spare cycles this week, I might be able to shepherd this in if you're prioritizing other things.

sunfishcode · 2019-04-18T16:23:55Z

Hi @rianhunter, sorry for being slow to respond here. If you're still able to help out here, that'd be a great!

Related to #7649 (point 5) and subset of #7799 * Update python.bc * Update output wasm sizes * Update wasm backend tests

Related to emscripten-core#7649 (point 5) and subset of emscripten-core#7799 * Update python.bc * Update output wasm sizes * Update wasm backend tests

sunfishcode · 2020-03-29T16:22:59Z

These changes are now best pursued by independent PRs such as #8467.

Related to emscripten-core#7649 (point 5) and subset of emscripten-core#7799 * Update python.bc * Update output wasm sizes * Update wasm backend tests

Update alltypes.h to match reference-sysroot types too.

4c346c2

This includes off_t, ino_t, and several other POSIX types.

sunfishcode added 2 commits January 2, 2019 22:17

Update library_syscall.js and struct stat.

284e8df

Update library_syscall.js to reflect that several types are i64 now. Struct stat is changing, so let's also update it to something with fewer odd legacy fields.

Fix the return type for lseek.

e2cdae4

rianhunter reviewed Jan 3, 2019

View reviewed changes

sunfishcode added 5 commits January 2, 2019 23:54

Fix the endianness of the offset argument in llseek.

d6aa926

Update the expected numbers for test_binaryen_metadce.

e13514c

Add a comment about the new stat layout.

6b6543d

Fix time functions for 64-bit time_t.

97724d7

Update the python.bc for 64-bit off_t.

79ad217

sunfishcode mentioned this pull request Jan 3, 2019

Update types to match the reference-sysroot types. jfbastien/musl#53

Merged

sunfishcode added 2 commits January 3, 2019 11:13

Update _Reg in alltypes.h too.

598e134

Fix the off_t argument in mmap2.

cb8966f

This was referenced Jan 3, 2019

Bump version to 1.38.22. emscripten-core/emscripten-fastcomp#249

Closed

Bump version to 1.38.22. emscripten-core/emscripten-fastcomp-clang#26

Closed

kripken reviewed Jan 3, 2019

View reviewed changes

sunfishcode added 5 commits January 3, 2019 13:23

Use l, h instead of lo, hi.

fc74ca4

Rename get64hilo to get64reversed.

5c857bf

Simplify assert conditions.

30ff717

Revert the changes to mmap2 calls.

073d256

Update more references to tv_sec and tv_usec to be 64-bit.

3476268

juj reviewed Jan 4, 2019

View reviewed changes

sunfishcode mentioned this pull request Jan 5, 2019

Add custom "emscripten_metadata" section to standalone WASM #7815

Merged

rianhunter mentioned this pull request Jan 7, 2019

Improvements to the Emscripten / WebAssembly API #7649

Closed

rianhunter mentioned this pull request Apr 18, 2019

Remove getdents64() API, implement opendir()/readdir() #8445

Closed

rianhunter mentioned this pull request Apr 20, 2019

Change ino_t/off_t to 64-bits #8467

Merged

kripken pushed a commit that referenced this pull request Apr 24, 2019

Change ino_t/off_t to 64-bits (#8467)

4e4a794

Related to #7649 (point 5) and subset of #7799 * Update python.bc * Update output wasm sizes * Update wasm backend tests

sbc100 closed this Jan 30, 2020

sbc100 reopened this Jan 31, 2020

sbc100 changed the base branch from incoming to master January 31, 2020 01:21

sunfishcode closed this Mar 29, 2020

sunfishcode deleted the match-reference-sysroot branch March 29, 2020 16:23

		@@ -1,4 +1,4 @@
		zlib version 1.2.5 = 4688, compile flags = 85
		zlib version 1.2.5 = 4688, compile flags = 149

		@@ -1 +1 @@
		"1.38.21"
		"1.38.22"

Conversation

sunfishcode commented Jan 3, 2019

Uh oh!

kripken commented Jan 3, 2019

Uh oh!

sunfishcode commented Jan 3, 2019

Uh oh!

rianhunter commented Jan 3, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rianhunter Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunfishcode commented Jan 3, 2019

Uh oh!

rianhunter commented Jan 3, 2019 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rianhunter commented Jan 5, 2019

Uh oh!

rianhunter commented Feb 11, 2019

Uh oh!

sunfishcode commented Apr 18, 2019

Uh oh!

sunfishcode commented Mar 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rianhunter commented Jan 3, 2019 via email •

edited

Loading

rianhunter Jan 3, 2019 •

edited

Loading