wasm: Store rlib metadata in wasm object files #120588

alexcrichton · 2024-02-02T21:47:52Z

The goal of this commit is to remove warnings using LLVM tip-of-tree wasm-ld. In llvm/llvm-project#78658 the wasm-ld LLD driver no longer looks at archive indices and instead looks at all the objects in archives. Previously lib.rmeta files were simply raw rustc metadata bytes, not wasm objects, meaning that wasm-ld would emit a warning indicating so.

WebAssembly targets previously passed --fatal-warnings to wasm-ld by default which meant that if Rust were to update to LLVM 18 then all wasm targets would not work. This immediate blocker was resolved in #120278 which removed --fatal-warnings which enabled a theoretical update to LLVM 18 for wasm targets. This current state is ok-enough for now because rustc squashes all linker output by default if it doesn't fail. This means, for example, that rustc squashes all the linker warnings coming out of wasm-ld about lib.rmeta files with LLVM 18. This again isn't a pressing issue because the information is all hidden, but it runs the risk of being annoying if another linker error were to happen and then the output would have all these unrelated warnings that couldn't be fixed.

Thus, this PR comes into the picture. The goal of this PR is to resolve these warnings by using the WebAssembly object file format on wasm targets instead of using raw rustc metadata. When I first implemented the rlib-in-objects scheme in #84449 I remember either concluding that wasm-ld would either include the metadata in the output or I thought we didn't have to do anything there at all. I think I was wrong on both counts as wasm-ld does not include the metadata in the final output unless the object is referenced and we do actually need to do something to resolve these warnings.

This PR updates the object file format containing rustc metadata on WebAssembly targets to be an actual WebAssembly file. To avoid bringing in any new dependencies I've opted to hand-code this encoding at this time. If the object gets more complicated though it'd probably be best to pull in wasmparser and wasm-encoder. For now though there's two adjacent functions reading/writing wasm.

The only caveat I know of with this is that if wasm-ld does indeed look at the object file then the metadata will be included in the final output. I believe the only thing that could cause that at this time is --whole-archive which I don't think is passed for rlibs. I would clarify that I'm not 100% certain about this, however.

rustbot · 2024-02-02T21:48:00Z

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

nikic · 2024-02-02T21:58:40Z

Is it possible to integrate this in create_object_file using the object crate? I believe it supports wasm as well.

bjorn3 · 2024-02-02T22:16:45Z

This again isn't a pressing issue because the information is all hidden

#119286 will show it.

compiler/rustc_codegen_ssa/src/back/metadata.rs

bjorn3 · 2024-02-02T22:24:35Z

Is it possible to integrate this in create_object_file using the object crate? I believe it supports wasm as well.

The object crate only supports parsing wasm, not emitting it. It also doesn't support relocatable wasm object files, only linked wasm modules. For this particular case it may still work as nothing is encoded in the linking section anyway.

compiler/rustc_codegen_ssa/src/back/metadata.rs

alexcrichton · 2024-02-09T10:47:28Z

Is it possible to integrate this in create_object_file using the object crate? I believe it supports wasm as well.

Oh good point, I did not know!

The object crate only supports parsing wasm, not emitting it.

Ah yes this appears to be the case.

It also looks like the support in object uses the wasmparser crate when enabled, and the wasm support in object isn't enabled by default. I've gone ahead and enabled the wasm feature which removes the need for special logic when reading the metadata section.

I'll also add some notes in the documentation about this for the writing half.

rustbot · 2024-02-09T10:59:06Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

alexcrichton · 2024-02-09T11:07:36Z

I've pushed an update to tidy checks to allow wasmparser, but if that's not something I should be doing here please let me know and I'd be ok restorign the older version which doesn't use the wasm feature of object which pulls in the wasmparser crate

rustbot · 2024-02-09T11:13:02Z

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

wesleywiser · 2024-02-09T16:19:26Z

src/tools/tidy/src/deps.rs

@@ -15,6 +15,7 @@ const LICENSES: &[&str] = &[
    "Apache-2.0 / MIT",
    "Apache-2.0 OR MIT",
    "Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT", // wasi license
+    "Apache-2.0 WITH LLVM-exception",                      // wasmparser license


Instead of adding the license here, could you add an exception for this crate?

rust/src/tools/tidy/src/deps.rs

Line 78 in 43c0d38

const EXCEPTIONS: ExceptionList = &[

Sorry to be nitpicky but I think this would help prevent us from depending on more Apache 2 only code without realizing it.

Oh no that definitely makes sense. I've moved it down there.

One point though is that I had to add an extra exception to the runtime checks loop. The object dependency is shared between rustc/libstd and the code here "isn't smart enough" to determine that libstd doesn't actually depend on wasmparser where rustc does, so I added some comments to that effect.

I'm also going to talk to some folks to try to get wasmparser relicensed with Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT which should resolve these concerns

and the code here "isn't smart enough" to determine that libstd doesn't actually depend on wasmparser where rustc does

Yet another thing that would be fixed by moving library/ to a separate cargo workspace...

src/tools/tidy/src/deps.rs

wesleywiser · 2024-02-09T19:52:40Z

Thanks Alex!

@bors r+

bors · 2024-02-09T19:52:43Z

📌 Commit 4f3348d has been approved by wesleywiser

It is now in the queue for this repository.

Instead substitute this with `AtomicUsize` which panics on overflow to catch any possible issues on 32-bit platforms. This is motivated by rust-lang/rust#120588 which is pulling `wasmparser` into the Rust compiler and `AtomicU64` is not available on all the platforms the compiler itself is built for. I plan on backporting this to the `0.118.x` release track as well which is what's used by the `object` crate currently to avoid the need for a whole bunch of cascading updates.

alexcrichton · 2024-02-12T14:54:19Z

For that failure bytecodealliance/wasm-tools#1412 was applied to the main branch of the wasmparser crate which I've backported to the 0.118.x release which is what the object crate is using, so this should be good for another try after updating rustc to using that version of the crate.

alexcrichton · 2024-02-20T17:16:58Z

Could I perhaps ping this to request a re-queue with bors?

The goal of this commit is to remove warnings using LLVM tip-of-tree `wasm-ld`. In llvm/llvm-project#78658 the `wasm-ld` LLD driver no longer looks at archive indices and instead looks at all the objects in archives. Previously `lib.rmeta` files were simply raw rustc metadata bytes, not wasm objects, meaning that `wasm-ld` would emit a warning indicating so. WebAssembly targets previously passed `--fatal-warnings` to `wasm-ld` by default which meant that if Rust were to update to LLVM 18 then all wasm targets would not work. This immediate blocker was resolved in rust-lang#120278 which removed `--fatal-warnings` which enabled a theoretical update to LLVM 18 for wasm targets. This current state is ok-enough for now because rustc squashes all linker output by default if it doesn't fail. This means, for example, that rustc squashes all the linker warnings coming out of `wasm-ld` about `lib.rmeta` files with LLVM 18. This again isn't a pressing issue because the information is all hidden, but it runs the risk of being annoying if another linker error were to happen and then the output would have all these unrelated warnings that couldn't be fixed. Thus, this PR comes into the picture. The goal of this PR is to resolve these warnings by using the WebAssembly object file format on wasm targets instead of using raw rustc metadata. When I first implemented the rlib-in-objects scheme in rust-lang#84449 I remember either concluding that `wasm-ld` would either include the metadata in the output or I thought we didn't have to do anything there at all. I think I was wrong on both counts as `wasm-ld` does not include the metadata in the final output unless the object is referenced and we do actually need to do something to resolve these warnings. This PR updates the object file format containing rustc metadata on WebAssembly targets to be an actual WebAssembly file. This enables the `wasm` feature of the `object` crate to be able to read the custom section in the same manner as other platforms, but currently `object` doesn't support writing wasm object files so a handwritten encoder is used instead. The only caveat I know of with this is that if `wasm-ld` does indeed look at the object file then the metadata will be included in the final output. I believe the only thing that could cause that at this time is `--whole-archive` which I don't think is passed for rlibs. I would clarify that I'm not 100% certain about this, however.

bjorn3 · 2024-02-20T17:42:51Z

@bors r=wesleywiser,bjorn3

bors · 2024-02-20T17:42:54Z

📌 Commit 6181f3a has been approved by wesleywiser,bjorn3

It is now in the queue for this repository.

bors · 2024-02-21T07:14:55Z

⌛ Testing commit 6181f3a with merge 7168c13...

bors · 2024-02-21T09:42:25Z

☀️ Test successful - checks-actions
Approved by: wesleywiser,bjorn3
Pushing 7168c13 to master...

rust-timer · 2024-02-21T10:57:21Z

Finished benchmarking commit (7168c13): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.1%	[3.0%, 3.3%]	4
Regressions ❌ (secondary)	1.5%	[0.2%, 2.9%]	33
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[3.0%, 3.3%]	4

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.5%	[2.3%, 4.9%]	3
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-4.8%	[-6.4%, -3.2%]	2
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 640.751s -> 651.363s (1.66%)
Artifact size: 308.59 MiB -> 310.99 MiB (0.78%)

alexcrichton · 2024-02-21T15:35:00Z

My best guess as to the source of the regression is that the wasm feature is now enabled in the object crate which adds an extra variant to much of its arms-of-things. The size of object::File doesn't change when toggling the wasm feature nor does std::mem::needs_drop, so even though this is my best guess I'm not certain why it would contribute towards a regression...

rylev · 2024-02-27T13:50:00Z

@alexcrichton I'm doing perf triage. Unfortunately, the regression here is significant enough to warrant some thought. The main concern are the regressions in the hello-world primary benchmark which has admittedly been a bit noisy lately. However, the regressions here do appear to be legitimate when compared to historical data.

The self profile comparison's on the results page don't really show anything conclusive either. 2 possible steps forward:

Open a PR reverting the changes and run a perf test on that to make sure that we see the predicated opposite perf improvement. If not, noise becomes much more likely of a explanation.
Run self-profiling locally and see if anything interesting pops out (you can find the directions at the top of the page here).

alexcrichton · 2024-02-27T21:58:56Z

Sounds good! I'll give the local testing a spin soon and failing that open a PR for a revert and measure that.

alexcrichton · 2024-02-28T22:07:27Z

Ok I've done local profiling and the results have yielded:

--------------------------------------------------------------------------------
-- File:function summary
--------------------------------------------------------------------------------
  Ir_______  file:function

< 1,249,595  ./elf/./elf/dl-lookup.c:
  1,026,935    _dl_lookup_symbol_x
    186,449    do_lookup_x
     36,211    check_match

<   126,032  ./elf/./elf/dl-addr.c:_dl_addr

<    66,269  ./elf/../sysdeps/x86_64/dl-machine.h:_dl_relocate_object

<    24,144  ./elf/./elf/do-rel.h:_dl_relocate_object

<    11,734  ./string/../sysdeps/x86_64/strcmp.S:strcmp

<     6,651  ./elf/../sysdeps/generic/dl-protected.h:do_lookup_x

<     5,912  ./elf/../sysdeps/generic/ldsodefs.h:
      3,695    do_lookup_x
      2,217    _dl_relocate_object

This gives rise to another hypothesis. I do not know the intricacies of the dynamic loader on Linux, but I can say for certain that on the previous commit before this PR librustc_driver had 14293 symbols (as reported by nm -g) and afterwards had 15206 symbols. This is because wasmparser, the crate pulled in here, has a lot of functions (e.g. a function-per-wasm-instruction and there's a lot of those). Almost none of those symbols are needed by rustc, but if the build process hasn't changed for librustc_driver since last I checked it pessimistically exported all crates in case any users of librustc_driver.so ended up needing them.

So my hypothesis is that this is because wasmparser has a bunch of symbols that rustc does not use. I don't know precisely if that would affect do_lookup_x but if I had to hazard a guess I would assume that _dl_lookup_symbol_x is probably doing a sort of linear search and may be needing to skip more symbols than before. That's just a guess though as I don't know how the dynamic loader works at a low-level.

I think it's probably safe to say that this isn't noise. As for what to do:

I'm happy to remove the wasmparser dependency here from this PR, it'll just involve adding a wad of parsing code and a bunch of special cases for wasm.
I don't know if there's a way to make these symbols private, if there is I'd be happy to make a PR to do that and see if the perf improves!

Or something else, happy to do what y'all think is best here! I'm also happy to take this discussion to Zulip as well.

rustbot assigned wesleywiser Feb 2, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 2, 2024

This was referenced Feb 2, 2024

[lld][WebAssembly] Match the ELF linker in transitioning away from archive indexes. llvm/llvm-project#78658

Merged

Remove --fatal-warnings on wasm targets #120278

Merged

bjorn3 reviewed Feb 2, 2024

View reviewed changes

compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated Show resolved Hide resolved

wesleywiser reviewed Feb 2, 2024

View reviewed changes

compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated Show resolved Hide resolved

compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated Show resolved Hide resolved

compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated Show resolved Hide resolved

alexcrichton force-pushed the wasm-rmeta-object branch from f377387 to 513258b Compare February 9, 2024 10:37

alexcrichton force-pushed the wasm-rmeta-object branch from 269b361 to 587c865 Compare February 9, 2024 10:50

This comment has been minimized.

Sign in to view

alexcrichton force-pushed the wasm-rmeta-object branch from 587c865 to 43c0d38 Compare February 9, 2024 11:06

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 9, 2024

wesleywiser requested changes Feb 9, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 9, 2024

alexcrichton force-pushed the wasm-rmeta-object branch from 43c0d38 to 74481b3 Compare February 9, 2024 18:43

bjorn3 reviewed Feb 9, 2024

View reviewed changes

src/tools/tidy/src/deps.rs Outdated Show resolved Hide resolved

wesleywiser approved these changes Feb 9, 2024

View reviewed changes

bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 9, 2024

alexcrichton mentioned this pull request Feb 10, 2024

Remove usage of AtomicU64 in wasmparser bytecodealliance/wasm-tools#1412

Merged

alexcrichton force-pushed the wasm-rmeta-object branch from 4f3348d to 47bb0de Compare February 12, 2024 14:53

alexcrichton requested a review from bjorn3 February 12, 2024 14:54

alexcrichton force-pushed the wasm-rmeta-object branch from 47bb0de to 6181f3a Compare February 20, 2024 17:31

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 20, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 21, 2024

bors merged commit 7168c13 into rust-lang:master Feb 21, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 21, 2024

rustbot added the perf-regression Performance regression. label Feb 21, 2024

alexcrichton deleted the wasm-rmeta-object branch February 21, 2024 15:24

matthiaskrgr mentioned this pull request Feb 22, 2024

Compiling tests for wasm64-unknown-unknown on latest Nightly results in linking error #121460

Closed

matthiaskrgr mentioned this pull request Oct 11, 2024

ICE: WellFormed .. has escaping bound vars, so it cannot be wrapped in a dummy binder. #131535

Closed

matthiaskrgr mentioned this pull request Nov 21, 2024

ICE: unexpected 'SelfCtor' in pattern, expected identifier #133272

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wasm: Store rlib metadata in wasm object files #120588

wasm: Store rlib metadata in wasm object files #120588

This comment has been minimized.

wasm: Store rlib metadata in wasm object files #120588

wasm: Store rlib metadata in wasm object files #120588

Conversation

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size