Skip to content
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

Set relocation_model to Pic on emscripten target #98149

Merged
merged 1 commit into from
Jun 25, 2022

Conversation

hoodmane
Copy link
Contributor

So we can support dynamically linking libraries with Emscripten (otherwise we need to use nightly and -Zbuild-std to rebuild std with relocations).
@sbc100

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 15, 2022
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2022
@bors
Copy link
Contributor

bors commented Jun 17, 2022

@hoodmane: 🔑 Insufficient privileges: Not in reviewers

@lcnr
Copy link
Contributor

lcnr commented Jun 20, 2022

r? rust-lang/compiler

@rust-highfive rust-highfive assigned jackh726 and unassigned lcnr Jun 20, 2022
@hoodmane
Copy link
Contributor Author

r? @petrochenkov

@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 22, 2022

wasm_base.rs has some comments relevant to this - https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/spec/wasm_base.rs#L111-L118

    // This has no effect in LLVM 8 or prior, but in LLVM 9 and later when
    // PIC code is implemented this has quite a drastic effect if it stays
    // at the default, `pic`. In an effort to keep wasm binaries as minimal
    // as possible we're defaulting to `static` for now, but the hope is
    // that eventually we can ship a `pic`-compatible standard library which
    // works with `static` as well (or works with some method of generating
    // non-relative calls and such later on).

So considering

In older versions of emscripten PIC objects was not compatible with non-PIC binaries, but today it should be

it looks like that the tradeoffs from this change are more or less:

  • Pros: Standard library can now be used for building dynamic libraries
  • Cons: Produced binaries are larger and slower (how significantly?)
    • Some size and performance can be recovered by -Crelocation-model=static, but it won't affect code coming from the standard library.

@petrochenkov
Copy link
Contributor

I guess the main alternative here is to build standard library specifically (libcore/libstd/etc) as PIC (in src/bootstrap) and keep the static default for everything else.

Then the tradeoffs will be:

  • Pros: Standard library can now be used for building dynamic libraries
  • Cons: Produced binaries are larger and slower (how significantly?), but only due standard library code
    • Size and performance can be recovered by using -Zbuild-std when really necessary.

@petrochenkov
Copy link
Contributor

I have no idea what alternative is preferable here.
Maybe you have some basic benchmarks on binary size and runtime performance, just to understand the order of magnitude?
@rustbot author

@rustbot 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 Jun 22, 2022
@hoodmane
Copy link
Contributor Author

Produced binaries are larger and slower

I think what @sbc100 said was that the final product should be about the same, but the object files will be larger and the compilation will be slower. This is a lot better than the final program being larger and slower.

@sbc100
Copy link

sbc100 commented Jun 22, 2022

Produced binaries are larger and slower

I think what @sbc100 said was that the final product should be about the same, but the object files will be larger and the compilation will be slower. This is a lot better than the final program being larger and slower.

Indeed, to the best of my knowledge, all of the PIC overhead can be removing by wasm-opt when run it is run on the output of wasm-ld. That is the say, the output of the linker will still contain an overhead but binaryen can then eliminate all of it (IIUC).

@hoodmane
Copy link
Contributor Author

I just built a library with PIC and with static. The PIC version is

2662027 bytes -- PIC
2661803 bytes -- static

so the static version come in 224 bytes smaller, which is a 0.008% difference. Kind of surprisingly similar size. Execution time is also the same within my large error margin. This isn't a very scientific test but I would believe that it makes no difference.

@sbc100
Copy link

sbc100 commented Jun 23, 2022

I just built a library with PIC and with static. The PIC version is

2662027 bytes -- PIC
2661803 bytes -- static

so the static version come in 224 bytes smaller, which is a 0.008% difference. Kind of surprisingly similar size. Execution time is also the same within my large error margin. This isn't a very scientific test but I would believe that it makes no difference.

Thanks for running the numbers!

If you have time/interest would you mind running both of those binaries through wasm-opt -O2.. I would hope they would yield the same results, and, if not, we can look into why not... but regardless it seems like -fPIC has a negligible effect, which is great news.

@hoodmane
Copy link
Contributor Author

After running wasm-opt -O2 on both, they differ by 204 bytes. wasm-opt reduces the pic file by 1887 bytes and the static file by 1867 bytes.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 23, 2022
@petrochenkov
Copy link
Contributor

Okay, seems fine to use pic by default then.
r=me after squashing commits.
@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2022
@hoodmane
Copy link
Contributor Author

Oops sorry. I will fix.

@hoodmane
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2022
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 24, 2022

📌 Commit ada2acc has been approved by petrochenkov

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 24, 2022
…nkov

Set relocation_model to Pic on emscripten target

So we can support dynamically linking libraries with Emscripten (otherwise we need to use nightly and `-Zbuild-std` to rebuild std with relocations).
`@sbc100`
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 25, 2022
…nkov

Set relocation_model to Pic on emscripten target

So we can support dynamically linking libraries with Emscripten (otherwise we need to use nightly and `-Zbuild-std` to rebuild std with relocations).
``@sbc100``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 25, 2022
…nkov

Set relocation_model to Pic on emscripten target

So we can support dynamically linking libraries with Emscripten (otherwise we need to use nightly and `-Zbuild-std` to rebuild std with relocations).
```@sbc100```
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jun 25, 2022
…nkov

Set relocation_model to Pic on emscripten target

So we can support dynamically linking libraries with Emscripten (otherwise we need to use nightly and `-Zbuild-std` to rebuild std with relocations).
````@sbc100````
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#96412 (Windows: Iterative `remove_dir_all`)
 - rust-lang#98126 (Mitigate MMIO stale data vulnerability)
 - rust-lang#98149 (Set relocation_model to Pic on emscripten target)
 - rust-lang#98194 (Leak pthread_{mutex,rwlock}_t if it's dropped while locked.)
 - rust-lang#98298 (Point to type parameter definition when not finding variant, method and associated item)
 - rust-lang#98311 (Reverse folder hierarchy)
 - rust-lang#98401 (Add tracking issues to `--extern` option docs.)
 - rust-lang#98429 (Use correct substs in enum discriminant cast)
 - rust-lang#98431 (Suggest defining variable as mutable on `&mut _` type mismatch in pats)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 45ef23d into rust-lang:master Jun 25, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jun 25, 2022
@hoodmane hoodmane deleted the emscripten-pic branch June 25, 2022 19:03
hoodmane added a commit to pyodide/pyodide that referenced this pull request Jun 27, 2022
Yesterday's nightly includes rust-lang/rust#98149 which allows us to remove the
PIC setting. This also means we won't ever have to put -Zbuild-std back in.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 16, 2022
…chenkov

Implement set_output_kind for Emscripten linker

This is on top of rust-lang#98149. See also the earlier discussion on rust-lang#98303. With this PR, a crate that specifies that it is a cdylib will compile to a working Emscripten dynamic library without adding any extra cargo, rustc, or linker flags and without fiddling with environment variables.

`@sbc100`

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 26, 2022
…chenkov

Implement set_output_kind for Emscripten linker

This is on top of rust-lang#98149. See also the earlier discussion on rust-lang#98303. With this PR, a crate that specifies that it is a cdylib will compile to a working Emscripten dynamic library without adding any extra cargo, rustc, or linker flags and without fiddling with environment variables.

`@sbc100`

r? `@petrochenkov`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants