emscripten: Upgrade emsdk to 3.1.68 #3962

kleisauke · 2024-10-11T08:45:10Z

In line with commit rust-lang/rust@2c38ecf.

Resolves: #3306.
Supersedes: #3569.
Cherry-picks a couple of changes from: #3307.

rustbot · 2024-10-11T08:45:16Z

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

hoodmane · 2024-10-11T08:56:35Z

Thanks @kleisauke it would be nice to see time_t finally get fixed...

juntyr · 2024-10-11T08:58:53Z

Could/should this PR keep the emcc version check to give a nice error at build time if a now-no-longer supported Emscripten is used?

hoodmane · 2024-10-11T10:26:20Z

I wouldn't want rust to generate an error when building with Emscripten 3.1.58 since that is the version that Pyodide is using with Python 3.12 and it would prevent us from upgrading. Though I suppose that it would just require us to use a patched rust/libc and we are already doing that, it would be nice to avoid additional patches.

hoodmane · 2024-10-11T10:26:49Z

But having a version check would be fine as long as there is an override.

juntyr · 2024-10-11T10:32:46Z

I was thinking about the previous check for 3.1.42 when the ABI change happened, which previous libc versions still supported (now there would be an ABI break in the opposite direction).

I definitely agree that other than that pretty much any emcc version has to be allowed since there is no current indicator which versions are ABI compatible and which aren’t

tgross35 · 2024-10-13T19:40:29Z

How backwards compatible is emsdk? I think putting this on the 0.2 stable branch should be okay, but would like to know if there is any expected breakage here.

kleisauke · 2024-10-15T10:23:53Z

Updated now that PR rust-lang/rust#131533 was landed. This is ready for review.

How backwards compatible is emsdk?

AFAIK, there were no ABI-breaking changes after Emscripten 3.1.42 (released at 2023/06/23), which was addressed in libc by commit 63b0d67 (but only when building with -Zbuild-std).

kleisauke · 2024-10-15T10:50:26Z

Could/should this PR keep the emcc version check to give a nice error at build time if a now-no-longer supported Emscripten is used?

I could address this within a follow-up PR, but perhaps its severity should be a warning instead? Simple programs that don't use filesystem operations will likely still work with Emscripten versions prior to 3.1.42. On the other hand, Emscripten 3.1.42 is already more than a year old.

hoodmane · 2024-10-15T11:39:37Z

AFAIK, there were no ABI-breaking changes after Emscripten 3.1.42

@kleisauke it would be really great if we could make some sort of tool to do ABI compatibility checks. For a static test, we could compare the contents of emscripten/src/struct_info_generated.json. Perhaps we could use emscripten/tools/maint/gen_struct_info.py to generate info about the layout of more standard library structs than just the ones that are in the default struct_info. For a dynamic test, maybe take the unistd test suite and build the tests as side modules with one emcc version then load them and call into them with a second emcc version.

This probably wouldn't be perfect, but it would be a lot better than the current method where we don't really know anything.

kleisauke · 2024-10-15T12:11:59Z

it would be really great if we could make some sort of tool to do ABI compatibility checks.

Agreed, we need to improve this. Note that a similar tool for this already exists in the repo. For example, when reverting the change in src/unix/linux_like/emscripten/align.rs within this PR locally, I see:

$ TARGET=wasm32-unknown-emscripten sh ./ci/install-rust.sh
$ ./ci/run-docker.sh wasm32-unknown-emscripten
[...]
bad max_align_t size: rust: 32 (0x20) != c 24 (0x18)
bad max_align_t align: rust: 16 (0x10) != c 8 (0x8)
size of max_align_t is 24 in C and 32 in Rust
thread 'main' panicked at /checkout/target/wasm32-unknown-emscripten/debug/build/libc-test-3c1238c857576107/out/main.rs:12:21:
some tests failed

However, that would only cover the types/functions/structs defined in src/unix/linux_like/emscripten/* and only works when EMSDK_VERSION is regularly updated.

Thinking about this further, how about changing EMSDK_VERSION to tot? This would ensure the tip-of-tree builds of Emscripten are tested in the CI, making it easier to identify potential ABI-breaking issues.

but it would be a lot better than the current method where we don't really know anything.

Note that wasm-vips regularly updates Rust and Emscripten to the latest versions, which is why the underlying issue in commit 63b0d67 and 7c952dc was resolved quickly. Based on that, I assumed there were no ABI-breaking changes after Emscripten 3.1.42, but I could be wrong.

bors · 2024-10-24T10:58:42Z

☔ The latest upstream changes (presumably #3993) made this pull request unmergeable. Please resolve the merge conflicts.

src/unix/linux_like/emscripten/mod.rs

tgross35 · 2024-11-06T19:15:41Z

I don't have any major concerns here but I'll defer to @hoodmane's review (I'll merge once you give it the checkmark)

hoodmane

Thanks @kleisauke generally looks good to me.

hoodmane · 2024-11-06T19:59:34Z

build.rs

-fn emcc_version_code() -> Option<u64> {
-    let output = std::process::Command::new("emcc")
-        .arg("-dumpversion")
-        .output()
-        .ok()?;
-    if !output.status.success() {
-        return None;
-    }
-
-    let version = String::from_utf8(output.stdout).ok()?;
-
-    // Some Emscripten versions come with `-git` attached, so split the
-    // version string also on the `-` char.
-    let mut pieces = version.trim().split(['.', '-']);
-
-    let major = pieces.next().and_then(|x| x.parse().ok()).unwrap_or(0);
-    let minor = pieces.next().and_then(|x| x.parse().ok()).unwrap_or(0);
-    let patch = pieces.next().and_then(|x| x.parse().ok()).unwrap_or(0);
-
-    Some(major * 10000 + minor * 100 + patch)
-}


Do we want to drop this entirely? Maybe we could adjust this to fail if Emscripten is below the minimum supported version?

This change was made based on the assumption that Rust requires Emscripten 3.1.42 or later. However, upon review, it appears that the compatibility note in rust-lang/rust#131689 indicates that versions prior 3.1.42 are still supported.

Let me drop commit a35f212 for now.

libc-test/build.rs

hoodmane · 2024-11-07T10:30:48Z

src/unix/linux_like/emscripten/align.rs

            #[allow(missing_debug_implementations)]
-            #[repr(align(16))]
+            #[repr(align(8))]


(Irrelevant remark.) The stack still has to be aligned to 16 bytes when you call a function even though maxalign is only 8. Discovering this wasted a lot of my time.

hoodmane · 2024-11-07T10:39:37Z

libc-test/build.rs

+            // FIXME: https://github.com/emscripten-core/emscripten/pull/14883
+            "SIG_IGN" => true,


What does this FIXME mean? Did that PR break this test? Why?

That PR did break the libc test, see e.g.:

RUNNING ALL TESTS bad SIG_IGN value at byte 0: rust: 1 (0x1) != c 254 (0xfe) bad SIG_IGN value at byte 1: rust: 0 (0x0) != c 255 (0xff) bad SIG_IGN value at byte 2: rust: 0 (0x0) != c 255 (0xff) bad SIG_IGN value at byte 3: rust: 0 (0x0) != c 255 (0xff) thread 'main' panicked at /checkout/target/wasm32-unknown-emscripten/debug/build/libc-test-890b1e2d4768b06a/out/main.rs:12:21: some tests failed

However, this FIXME can probably not fixed according to
https://github.com/emscripten-core/emscripten/pull/14883/files#r892912370.

I'll change this to:

Suggested change

// FIXME: https://github.com/emscripten-core/emscripten/pull/14883

"SIG_IGN" => true,

// `SIG_IGN` has been changed to -2 since 1 is a valid function address

// https://github.com/emscripten-core/emscripten/pull/14883

"SIG_IGN" => true,

Note that other implementations have a similar issue, see e.g:

libc/libc-test/build.rs

Line 1198 in e4c7acc

"SIG_DFL" | "SIG_ERR" | "SIG_IGN" => true, // sighandler_t weirdness

Maybe there should be a constant somewhere that indicates what the value of SIG_IGN is on the platform... But that would be best dealt with in a followup.

tgross35

Just a couple small nits before merge. Thanks @hoodmane for taking a look.

tgross35 · 2024-11-13T00:09:32Z

src/unix/linux_like/emscripten/align.rs

+            // Lowered from 16 to 8 bytes in
+            // https://github.com/llvm/llvm-project/commit/d1a96e906cc03a95cfd41a1f22bdda92651250c7


This can just go in the commit message

Split into commit 52fa133 and moved the time_t description to the commit message as well.
https://github.com/rust-lang/libc/compare/b6a957f2ec9f24a843d04ec4313f3a4634944072..a37fa9d267ddb3c7cbc9504ac730a7ed6fc11f59

tgross35 · 2024-11-13T00:13:06Z

src/unix/linux_like/emscripten/mod.rs

-pub const S_IEXEC: mode_t = 0o0100;
-pub const S_IWRITE: mode_t = 0o0200;
-pub const S_IREAD: mode_t = 0o0400;
+pub const S_IEXEC: mode_t = 64;
+pub const S_IWRITE: mode_t = 128;
+pub const S_IREAD: mode_t = 256;


These look like they are octal in the C source, could they stay that way?

I reverted this part, I can't remember why I did this in the first place.

See: emscripten-core/emscripten@6416c35.

See: emscripten-core/emscripten@15f7632.

See: emscripten-core/emscripten@655ad88.

See: llvm/llvm-project@d1a96e9.

In line with commit rust-lang/rust@2c38ecf. Notable changes: - `time_t` changed to 64-bit. emscripten-core/emscripten@c8857a6

See: emscripten-core/emscripten@6416c35. (backport <rust-lang#3962>) (cherry picked from commit 01c72ee)

See: emscripten-core/emscripten@15f7632. (backport <rust-lang#3962>) (cherry picked from commit 6670ab2)

See: emscripten-core/emscripten@655ad88. (backport <rust-lang#3962>) (cherry picked from commit 0df7c93)

See: llvm/llvm-project@d1a96e9. (backport <rust-lang#3962>) (cherry picked from commit 99035d7)

In line with commit rust-lang/rust@2c38ecf. Notable changes: - `time_t` changed to 64-bit. emscripten-core/emscripten@c8857a6 (backport <rust-lang#3962>) (cherry picked from commit 3e82531)

rustbot assigned JohnTitor Oct 11, 2024

rustbot added the S-waiting-on-review label Oct 11, 2024

kleisauke mentioned this pull request Oct 11, 2024

Emscripten metadata file size is wrong rust-lang/rust#131467

Closed

tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Oct 13, 2024

kleisauke force-pushed the update-emscripten-version branch 2 times, most recently from 63bbd45 to 839239d Compare October 15, 2024 10:22

kleisauke changed the title ~~Draft: emscripten: Use the latest emsdk 3.1.68~~ emscripten: Upgrade emsdk to 3.1.68 Oct 15, 2024

kleisauke marked this pull request as ready for review October 15, 2024 10:23

rgonzalezfluendo mentioned this pull request Oct 23, 2024

Add support to gst-plugins-rs fluendo/gst.wasm#56

Open

kleisauke force-pushed the update-emscripten-version branch from 839239d to 073dbdf Compare October 24, 2024 12:29

tgross35 reviewed Nov 6, 2024

View reviewed changes

src/unix/linux_like/emscripten/mod.rs Outdated Show resolved Hide resolved

kleisauke force-pushed the update-emscripten-version branch from 073dbdf to a35f212 Compare November 7, 2024 10:03

hoodmane approved these changes Nov 7, 2024

View reviewed changes

kleisauke force-pushed the update-emscripten-version branch from a35f212 to b6a957f Compare November 7, 2024 11:10

tgross35 requested changes Nov 13, 2024

View reviewed changes

tgross35 added S-waiting-on-author and removed S-waiting-on-review labels Nov 13, 2024

rustbot added S-waiting-on-review and removed S-waiting-on-author labels Nov 13, 2024

kleisauke added 5 commits November 13, 2024 17:34

emscripten: Remove aio.h usage

01c72ee

See: emscripten-core/emscripten@6416c35.

emscripten: Remove sys/sysctl.h usage

6670ab2

See: emscripten-core/emscripten@15f7632.

emscripten: Remove Linux-specific implementations

0df7c93

See: emscripten-core/emscripten@655ad88.

emscripten: Lower max_align_t from 16 to 8 bytes

99035d7

See: llvm/llvm-project@d1a96e9.

emscripten: Upgrade emsdk to 3.1.68

3e82531

In line with commit rust-lang/rust@2c38ecf. Notable changes: - `time_t` changed to 64-bit. emscripten-core/emscripten@c8857a6

tgross35 force-pushed the update-emscripten-version branch from a37fa9d to 3e82531 Compare November 14, 2024 00:34

rustbot added O-unix O-wasm labels Nov 14, 2024

tgross35 approved these changes Nov 14, 2024

View reviewed changes

tgross35 enabled auto-merge November 14, 2024 00:34

tgross35 removed the O-wasm label Nov 14, 2024

tgross35 mentioned this pull request Nov 14, 2024

Fix: time_t is 64 bits on Emscripten #3569

Closed

tgross35 added this pull request to the merge queue Nov 14, 2024

Merged via the queue into rust-lang:main with commit 71e623d Nov 14, 2024
43 checks passed

This was referenced Nov 14, 2024

Fix the definition of sigevent on FreeBSD and Linux #3630

Merged

Move common Linux ptrace definitions to linux_like/mod.rs #4030

Open

feat: add aio for solarish #4033

Merged

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Nov 15, 2024

emscripten: Remove aio.h usage

58dcc8a

See: emscripten-core/emscripten@6416c35. (backport <rust-lang#3962>) (cherry picked from commit 01c72ee)

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Nov 15, 2024

emscripten: Remove sys/sysctl.h usage

7e5427f

See: emscripten-core/emscripten@15f7632. (backport <rust-lang#3962>) (cherry picked from commit 6670ab2)

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Nov 15, 2024

emscripten: Remove Linux-specific implementations

51248a5

See: emscripten-core/emscripten@655ad88. (backport <rust-lang#3962>) (cherry picked from commit 0df7c93)

tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Nov 15, 2024

emscripten: Lower max_align_t from 16 to 8 bytes

eb821a3

See: llvm/llvm-project@d1a96e9. (backport <rust-lang#3962>) (cherry picked from commit 99035d7)

tgross35 mentioned this pull request Nov 15, 2024

[0.2] Backports #4039

Merged

tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Nov 16, 2024

hoodmane mentioned this pull request Dec 3, 2024

Bump rust toolchain version pyodide/pyodide#5237

Merged

RalfJung mentioned this pull request Dec 4, 2024

How to deal with breaking changes on platform ? [BSDs related] #570

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

emscripten: Upgrade emsdk to 3.1.68 #3962

emscripten: Upgrade emsdk to 3.1.68 #3962

kleisauke commented Oct 11, 2024 •

edited

Loading

rustbot commented Oct 11, 2024

hoodmane commented Oct 11, 2024

juntyr commented Oct 11, 2024

hoodmane commented Oct 11, 2024

hoodmane commented Oct 11, 2024

juntyr commented Oct 11, 2024

tgross35 commented Oct 13, 2024

kleisauke commented Oct 15, 2024

kleisauke commented Oct 15, 2024

hoodmane commented Oct 15, 2024

kleisauke commented Oct 15, 2024

bors commented Oct 24, 2024

tgross35 commented Nov 6, 2024

hoodmane left a comment

hoodmane Nov 6, 2024

kleisauke Nov 7, 2024

hoodmane Nov 7, 2024

hoodmane Nov 7, 2024

kleisauke Nov 7, 2024

hoodmane Nov 7, 2024 •

edited

Loading

tgross35 left a comment

tgross35 Nov 13, 2024

kleisauke Nov 13, 2024

tgross35 Nov 13, 2024 •

edited

Loading

kleisauke Nov 13, 2024

		// FIXME: https://github.com/emscripten-core/emscripten/pull/14883
		"SIG_IGN" => true,

		// Lowered from 16 to 8 bytes in
		// https://github.com/llvm/llvm-project/commit/d1a96e906cc03a95cfd41a1f22bdda92651250c7

emscripten: Upgrade emsdk to 3.1.68 #3962

emscripten: Upgrade emsdk to 3.1.68 #3962

Conversation

kleisauke commented Oct 11, 2024 • edited Loading

rustbot commented Oct 11, 2024

hoodmane commented Oct 11, 2024

juntyr commented Oct 11, 2024

hoodmane commented Oct 11, 2024

hoodmane commented Oct 11, 2024

juntyr commented Oct 11, 2024

tgross35 commented Oct 13, 2024

kleisauke commented Oct 15, 2024

kleisauke commented Oct 15, 2024

hoodmane commented Oct 15, 2024

kleisauke commented Oct 15, 2024

bors commented Oct 24, 2024

tgross35 commented Nov 6, 2024

hoodmane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoodmane Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

tgross35 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kleisauke commented Oct 11, 2024 •

edited

Loading

hoodmane Nov 7, 2024 •

edited

Loading

tgross35 Nov 13, 2024 •

edited

Loading