Skip to content

Backport of Wasm 64-bit table fix #180

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

adambratschikaye
Copy link

The wasm64-unknown-unkown target produces invalid Wasm modules due to an issue with 64-bit tables that was fixed in llvm#107780. This is a backport of that fix which gets the wasm64 target working again.

Here are steps to reproduce the issue:

❯ rustc +nightly --version
rustc 1.85.0-nightly (a224f3807 2024-12-09)

❯ cargo new --bin foo

❯ cd foo

❯ cargo +nightly build -Z build-std=std,panic_abort --target wasm64-unknown-unknown

❯ wasm-objdump -j Elem -x target/wasm64-unknown-unknown/debug/foo.wasm | head

foo.wasm:       file format wasm 0x1
module name: <foo-87b5668ca08bcff0.wasm>

Section Details:

Elem[1]:
 - segment[0] flags=0 table=0 count=91 - init i64=94235877441537
  - elem[94235877441537] = ref.func:3 <_ZN3foo4main17h3e4566f0ffb33d02E>
  - elem[94235877441538] = ref.func:5 <_ZN4core3ops8function6FnOnce40call_once$u7b$$u7b$vtable.shim$u7d$$u7d$17ha696e16b756653ccE>
Here we can see the init expression for the table is reading uninitialized memory.

cc @alexcrichton @lwshang

…lvm#107780)

In the case of `--wasm64` we were setting the type of the init expression
to be 64-bit but were only setting the low 32-bits of the value (by
assigning to Int32).

Fixes: emscripten-core/emscripten#22538
@alexcrichton
Copy link
Member

I'll also note here that @adambratschikaye is also, in parallel, requesting a backport to LLVM's release branch as well.

(otherwise 👍 from me)

@dianqk
Copy link
Member

dianqk commented Dec 13, 2024

We can wait for the next minor version or llvm#119723 is merged. :)

@dianqk dianqk closed this Dec 13, 2024
@alexcrichton
Copy link
Member

@dianqk could I perhaps ask why? Are backports not allowed to Rust's LLVM fork any more when they're landed upstream?

The backport PR seems like it might not make the theoretical 19.1.6 window which could delay this all the way to 20.1.x which could take quite some time.

@nikic
Copy link

nikic commented Dec 16, 2024

If upstream declines the backport, we can consider a downstream backport.

@alexcrichton
Copy link
Member

The response above seems to indicate that this won't be considered at all, and I've historically been under the impression that there aren't that many roadblocks backporting LLVM patches to rustc's fork of LLVM, but has that changed? LLVM's merge/release cycle is generally quite slow which means that if a fix is desired in rustc's LLVM then there's no way to get it out to users in a faster many any more?

I also ask this because I'm pretty surprised by this myself. I was under the impression that this backport process isn't a burden on any other maintainers because the backports and PRs are being managed by others (e.g. @adambratschikaye here) and so in theory "only" reviews are required. Are there other pressures in play though which lead to rejecting changes like this? If so could they be documented in the rustc-dev-guide?

@nikic
Copy link

nikic commented Dec 16, 2024

See https://rustc-dev-guide.rust-lang.org/backend/updating-llvm.html for documentation on LLVM updates. We are currently in the Backports (upstream supported) process, as LLVM 19 is still supported.

We prefer backports to go upstream first if possible, both as a matter of due diligence, and to avoid divergence. That way everyone gets the patch, not just Rust built against bundled LLVM.

That's the usual process. Of course we can always deviate from it if necessary. For example, if the issue is time-critical because waiting for an upstream backport may cause us to miss the beta backport window and ship a stable regression.

@alexcrichton
Copy link
Member

This is somewhat straying off-topic at this point so if it's better to discuss this elsewhere let me know, but otherwise I'm sort of tangentially asking questions about the policy. Instead I can directly ask what I'm thinking: why is the current policy throwing up more of a gauntlet beyond "it's upstream in main"? I understand the rationale to never carry Rust-specific changes in this fork, but once something is in upstream LLVM (which is already a challenging enough task for bug fixes in niche platforms like this IMO) why is it necessary to have even more requirements such as also requiring it to be in the upstream backport branches too?

While I can sort of understand that logic from a "this is a major platform" perspective, e.g. x64, it's otherwise just actively harmful to other platforms like wasm64. No distro or other distribution of Rust is shipping wasm64 targets right now, even rustc isn't shipping wasm64 targets. Worrying about exact LLVM compatibility for a niche target that no one uses outside of the rustup-distributed channels feels to me like it's throwing up bureacracy and roadblocks to avoid considering changes.

I do understand as well that each change can't be micro-analyzed and have a case-by-case decision being made, though. That's too much to ask of maintainers! At the same time though if this is going to be closed with a ":)" outright I would at least personally advocate for a change in policy. I would personally encourage an amendment to include consideration of the tier of the target affected. For example if a tier 1 target is affected then it can be required to be in other LLVM distribution channels and 2-3 wouldn't have such a requirement. Personally though I'd advocate for dropping the "must be upstream in llvm's backport channel" requirement if it's already handled when the LLVM upstream release is unsupported.

@nikic
Copy link

nikic commented Dec 16, 2024

As said, the main purpose of requesting upstream backports is that other users of LLVM (including Rust linked against distro LLVM) also get the patch. I agree that this consideration is not particularly important for wasm64 in particular, but it is relevant for the majority of backports we're interested in. And this isn't really a tier 1 distinction, this is also relevant for most tier 2 targets. As such, I think this is a sensible default policy.

It's certainly not a hard policy -- we can always do something on a case by case basis, but in that case the backport PR should actually explain why...

For this specific PR, I can totally see the argument that having an upstream backport for a wasm64 fix is not super important -- but for the same reason, I also don't see why a 3 month old fix for a tier 3 target suddenly becomes time-critical enough that waiting for the next LLVM patch release (which is tomorrow) becomes overly onerous.

@nikic
Copy link

nikic commented Dec 16, 2024

I'll also add that the intention here really isn't to add more work. The backport process from your perspective should really boil down to just "request an upstream backport" -- the rest should happens as a matter of course when Rust pulls in the next LLVM patch release.

@alexcrichton
Copy link
Member

I definitely don't mean to assign a sense of urgency to this PR by any means. I talked with @lwshang this morning and they indicated it'd be nice to get this in sooner rather than later, so I've decided to push on this a bit. I'm trying to reconcile the process since this isn't how things used to be done but it's been a long time since I was doing these things (hence the reconciling in real time here for me)

I at least personally still feel that platforms which have the most to benefit from a faster backport process, niche (ish) platforms that are likely tier 3, have the most to lose from adding more process in layers to getting something fixed. By virtue of being tier 3 landing in LLVM probably takes awhile because there probably isn't a large body of developers working on the platforms. Landing a backport in LLVM probably takes longer yet as there aren't that many people watching it. Landing then in rust-lang/llvm-project takes yet longer and then on rust-lang/rust. Basically that's quite a lot of PRs for a change.

I agree that the change will eventually propagate and there's no need to proactively do work, but closing this PR with no discussion feels like it's actively saying that even if someone is willing to do the work it's not welcome. By being a tier 3 niche platform it's by definition not really urgent to most folks, but that doesn't make it any less urgent for those who want to rely on the fix. Waiting for this change to naturally propagate could take on the order of months which drags down downstream development.

In the end though I'm just a bystander and if this style of backport is no longer desired I'll adjust to recommending what the docs refer to. I don't feel like the current status quo is a good balance of concerns, but I also won't champion the cause to try to change things.

@dianqk
Copy link
Member

dianqk commented Dec 17, 2024

At the same time though if this is going to be closed with a ":)" outright I would at least personally advocate for a change in policy.

@alexcrichton
I apologize for not linking the relevant documents when closing. And thank you for pointing out some of the current concerns.

I'm not sure how we should balance these, either. But I think we're losing an important capability of forking: our ability to make updates more flexibly regardless of upstream behavior.

We prefer backports to go upstream first if possible, both as a matter of due diligence, and to avoid divergence. That way everyone gets the patch, not just Rust built against bundled LLVM.

@nikic
I agree. But once a change is merged into the upstream main branch, everyone will eventually get it, so I think Rust's bundled LLVM can move faster.


In any case, I think we should raise a topic on Zulip to improve this process. I will organize this in this week.
I agree that we can consider to backport it once it's merged into the main branch, but IMO, the upstreams two-week version update cycle isn't particularly long.

@dianqk dianqk reopened this Dec 17, 2024
@dianqk
Copy link
Member

dianqk commented Dec 17, 2024

For this specific PR, I can totally see the argument that having an upstream backport for a wasm64 fix is not super important -- but for the same reason, I also don't see why a 3 month old fix for a tier 3 target suddenly becomes time-critical enough that waiting for the next LLVM patch release (which is tomorrow) becomes overly onerous.

I guess this is because this issue wasn't initially reported in Rust. By the time we found this issue in Rust, we noticed that upstream had already fixed it.

@lwshang
Copy link

lwshang commented Dec 17, 2024

but IMO, the upstreams two-week version update cycle isn't particularly long.

Yes, two weeks is indeed short, but it’s a bit more tricky at the moment.

LLVM v19.1.5 is the latest release. As per LLVM release schedule, v19.1.6 may happen "if necessary". Even if there will be a v19.1.6, it will be released tomorrow (2 weeks after v19.1.5). And I'm not optimistic that llvm#119717 will be merged before then.

If we miss the v19.1.6 train, then we may have to wait for v20.1.X. If I calculated correctly, v20.1.0-rc1 will be released on 1/31/2025 (3 days after the 4th Tue in January). That's not too late.
But will we be upgrading the rust-lang/llvm-project to v20.1.0-rc1 shortly after that?

@cuviper
Copy link
Member

cuviper commented Dec 17, 2024

If it doesn't make it to a 19 point release upstream, we can still backport it. We will try 20 soon after it branches, but that's not always smooth...

@dianqk
Copy link
Member

dianqk commented Dec 17, 2024

If we miss the v19.1.6 train, then we may have to wait for v20.1.X. If I calculated correctly, v20.1.0-rc1 will be released on 1/31/2025 (3 days after the 4th Tue in January). That's not too late.

I will merge this PR if we miss.

But will we be upgrading the rust-lang/llvm-project to v20.1.0-rc1 shortly after that?

Due to frequent issues encountered during merging, this cannot be guaranteed. Also, to avoid the frequent backporting caused by merges during the RC period, I may prefer to slightly delay the upgrading timing.

@nikic
Copy link

nikic commented Dec 17, 2024

but IMO, the upstreams two-week version update cycle isn't particularly long.

Yes, two weeks is indeed short, but it’s a bit more tricky at the moment.

LLVM v19.1.5 is the latest release. As per LLVM release schedule, v19.1.6 may happen "if necessary".

Okay, I can see where the confusion here comes from. LLVM 19 is the first release to test an extended release cycle. It should go up to 19.1.8, right to the start of the LLVM 20 release cycle.

Even if there will be a v19.1.6, it will be released tomorrow (2 weeks after v19.1.5). And I'm not optimistic that llvm#119717 will be merged before then.

The PR is approved and has passing CI -- there is no reason to assume it will not be merged. (And indeed it has been merged.)

@alexcrichton
Copy link
Member

I didn't realize myself that LLVM's release cycle was being changed to be more rapid for patch releases as well, so if there's a bi-weekly cadence of patch releases that definitely reduces the need for having other processes/allowances for backports direct-to-rustc. Thanks regardless though for considering this and indulging my questions/thoughts!

@dianqk
Copy link
Member

dianqk commented Dec 18, 2024

In any case, I think we should raise a topic on Zulip to improve this process. I will organize this in this week.

It seems we no longer need this discussion, as we've agreed that LLVM's bi-weekly update cycle is reasonable.

@dianqk dianqk closed this Dec 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2024
Update to LLVM 19.1.6

Fixes the wasm64 build failure reported at rust-lang/llvm-project#180.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2024
Update to LLVM 19.1.6

Fixes the wasm64 build failure reported at rust-lang/llvm-project#180.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 20, 2024
Update to LLVM 19.1.6

Fixes the wasm64 build failure reported at rust-lang/llvm-project#180.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Dec 23, 2024
Update to LLVM 19.1.6

Fixes the wasm64 build failure reported at rust-lang/llvm-project#180.
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.

7 participants