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

Handle .init_array link_section specially on wasm #121533

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

ratmice
Copy link
Contributor

@ratmice ratmice commented Feb 24, 2024

Given that wasm-ld now has support for .init_array, it appears we can easily implement that section by falling through to the normal path rather than taking the typical custom_section path for wasm.

The wasm-ld appears to have a bunch of limitations. Only one static with the link_section in a crate or else you hit the fatal error in the link above "only one .init_array section fragment supported". They do not get merged.

You can still call multiple constructors by setting it to an array.

unsafe extern "C" fn ctor() {
    println!("foo");
}
#[used]
#[link_section = ".init_array"]
static FOO: [unsafe extern "C" fn(); 2] = [ctor, ctor];

Another issue appears to be that if crate A depends on crate B, but A doesn't call any symbols from B and B doesn't #[export_name = ...] any symbols, then crate B's constructor will not be called. The workaround to this is to provide an exported symbol in crate B.

@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2024

r? @estebank

rustbot has assigned @estebank.
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

@rustbot 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 24, 2024
if attrs
.link_section
.map(|link_section| !link_section.as_str().starts_with(".init_array"))
.unwrap_or(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this could just be unwrap() because of the early return on line 158-159

@bjorn3
Copy link
Member

bjorn3 commented Feb 24, 2024

Another issue appears to be that if crate A depends on crate B, but A doesn't call any symbols from B and B doesn't #[export_name = ...] any symbols, then crate B's constructor will not be called. The workaround to this is to provide an exported symbol in crate B.

Does #[used] or #[used(linker)] on the static fix that?

@ratmice
Copy link
Contributor Author

ratmice commented Feb 24, 2024

Another issue appears to be that if crate A depends on crate B, but A doesn't call any symbols from B and B doesn't #[export_name = ...] any symbols, then crate B's constructor will not be called. The workaround to this is to provide an exported symbol in crate B.

Does #[used] or #[used(linker)] on the static fix that?

No, neither helps, nor does passing --no-gc-sections, I can confirm that the used attributes were present in the .ll emitted,
and my guess is that it is a problem in the linker in the generation of the __wasm_call_ctors function by the linker.

@bjorn3
Copy link
Member

bjorn3 commented Feb 24, 2024

And adding --whole-archive?

@ratmice
Copy link
Contributor Author

ratmice commented Feb 24, 2024

And adding --whole-archive?

Doesn't appear to help either.

I should probably mention if it helps I'm using the wasi-root for the current download version of wasi-sdk.

@apiraino
Copy link
Contributor

r? compiler

@rustbot rustbot assigned nnethercote and unassigned estebank Jun 13, 2024
// show up, reject it here.
// For the wasm32 target statics with `#[link_section]` other than `.init_array`
// are placed into custom sections of the final output file, but this isn't link
// custom sections of other executable formats. Namely we can only embed a list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but might as well fix it: "this isn't link custom sections..." is nonsensical phrasing. I'm not sure what the correct words would be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe s/link/like?

@nnethercote
Copy link
Contributor

This seems reasonable to me but I don't know much about wasm. @bjorn3, are you willing to review/approve this? Also, is anything from the @bjorn3/@ratmice worth putting into a comment somewhere?

@ratmice
Copy link
Contributor Author

ratmice commented Jun 14, 2024

I'm not really sure any of the things discussed are worth putting into a comment, in particular they are limitations of the wasm-ld linker, and could change easily without notice. I had considered putting it into a comment. But it really feels like something which is just bound to become stale and out of date if at some point in the future if the linker is ever fixed.

It does feel like that stuff could/should be documented somewhere, but perhaps the best place for it to be documented is by linker itself?

@nnethercote
Copy link
Contributor

What about a comment like this: "the wasm-ld linker currently has $LIMITATION, so we work around it by doing $X. If this limitation is lifted in the future we can instead do $Y"? I can imagine somebody in the future finding that comment extremely useful, in a Chesterton's Fence kind of way.

@ratmice
Copy link
Contributor Author

ratmice commented Jun 14, 2024

I tried to fix up most of the review comments, and added a new comment like you asked.
I'm still a bit uncertain about really where this comment really belongs, but at least it is somewhere, hopefully that is ok.

@nnethercote
Copy link
Contributor

The new comment is good, thank you. I will give @bjorn3 a chance to add any extra comments, but if they don't say anything by early next week I think this is ok to merge.

@bors delegate=ratmice

@bors
Copy link
Contributor

bors commented Jun 15, 2024

✌️ @ratmice, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

@ratmice
Copy link
Contributor Author

ratmice commented Jul 18, 2024

Forgot about this one for a bit, so it has been a bit longer than a week.
Gopefully I understand bors instructions correctly. @bors r=@nnethercote

@bors
Copy link
Contributor

bors commented Jul 18, 2024

📌 Commit 1de046f has been approved by nnethercote

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 18, 2024
…ethercote

Handle .init_array link_section specially on wasm

Given that wasm-ld now has support for [.init_array](https://github.com/llvm/llvm-project/blob/8f2bd8ae68883592a333f4bdbed9798d66e68630/llvm/lib/MC/WasmObjectWriter.cpp#L1852), it appears we can easily implement that section by falling through to the normal path rather than taking the typical custom_section path for wasm.

The wasm-ld appears to have a bunch of limitations. Only one static with the `link_section` in a crate or else you hit the fatal error in the link above "only one .init_array section fragment supported". They do not get merged.

You can still call multiple constructors by setting it to an array.

```
unsafe extern "C" fn ctor() {
    println!("foo");
}
#[used]
#[link_section = ".init_array"]
static FOO: [unsafe extern "C" fn(); 2] = [ctor, ctor];
```

Another issue appears to be that if crate *A* depends on crate *B*, but *A* doesn't call any symbols from *B* and *B* doesn't `#[export_name = ...]` any symbols, then crate *B*'s constructor will not be called.  The workaround to this is to provide an exported symbol in crate *B*.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#121533 (Handle .init_array link_section specially on wasm)
 - rust-lang#123196 (Add Process support for UEFI)
 - rust-lang#127621 (Rewrite and rename `issue-22131` and `issue-26006` `run-make` tests to rmake)
 - rust-lang#127928 (Migrate `lto-smoke-c` and `link-path-order` `run-make` tests to rmake)
 - rust-lang#127935 (Change `binary_asm_labels` to only fire on x86 and x86_64)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#121533 (Handle .init_array link_section specially on wasm)
 - rust-lang#123196 (Add Process support for UEFI)
 - rust-lang#127621 (Rewrite and rename `issue-22131` and `issue-26006` `run-make` tests to rmake)
 - rust-lang#127928 (Migrate `lto-smoke-c` and `link-path-order` `run-make` tests to rmake)
 - rust-lang#127935 (Change `binary_asm_labels` to only fire on x86 and x86_64)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#121533 (Handle .init_array link_section specially on wasm)
 - rust-lang#127825 (Migrate `macos-fat-archive`, `manual-link` and `archive-duplicate-names` `run-make` tests to rmake)
 - rust-lang#127891 (Tweak suggestions when using incorrect type of enum literal)
 - rust-lang#127902 (`collect_tokens_trailing_token` cleanups)
 - rust-lang#127928 (Migrate `lto-smoke-c` and `link-path-order` `run-make` tests to rmake)
 - rust-lang#127935 (Change `binary_asm_labels` to only fire on x86 and x86_64)
 - rust-lang#127953 ([compiletest] Search *.a when getting dynamic libraries on AIX)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 986d6bf into rust-lang:master Jul 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
Rollup merge of rust-lang#121533 - ratmice:wasm_init_fini_array, r=nnethercote

Handle .init_array link_section specially on wasm

Given that wasm-ld now has support for [.init_array](https://github.com/llvm/llvm-project/blob/8f2bd8ae68883592a333f4bdbed9798d66e68630/llvm/lib/MC/WasmObjectWriter.cpp#L1852), it appears we can easily implement that section by falling through to the normal path rather than taking the typical custom_section path for wasm.

The wasm-ld appears to have a bunch of limitations. Only one static with the `link_section` in a crate or else you hit the fatal error in the link above "only one .init_array section fragment supported". They do not get merged.

You can still call multiple constructors by setting it to an array.

```
unsafe extern "C" fn ctor() {
    println!("foo");
}
#[used]
#[link_section = ".init_array"]
static FOO: [unsafe extern "C" fn(); 2] = [ctor, ctor];
```

Another issue appears to be that if crate *A* depends on crate *B*, but *A* doesn't call any symbols from *B* and *B* doesn't `#[export_name = ...]` any symbols, then crate *B*'s constructor will not be called.  The workaround to this is to provide an exported symbol in crate *B*.
@ratmice ratmice deleted the wasm_init_fini_array branch July 31, 2024 21:00
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.

7 participants