Rollup of 2 pull requests#156574
Closed
JonathanBrouwer wants to merge 9 commits into
Closed
Conversation
Add lint againts invalid runtime symbol definitions
This PR adds a deny-by-default lint againts invalid runtime symbol definitions, those runtime symbols are assumed and used by `core`[^1] and `rustc` with a specific definition.
We have had multiple reports of users tripping over `std` symbols (addressed in a future PR):
- [Why does `#[no_mangle] fn open() {}` make `cargo t` hang?](https://users.rust-lang.org/t/why-does-no-mangle-fn-open-make-cargo-t-hang/103423)
- [Pointer becomes misaligned in test with `no_mangle`](https://users.rust-lang.org/t/pointer-becomes-misaligned-in-test-with-no-mangle/126580)
This PR is a second attempt after rust-lang#146505, where T-lang had [some reservations](rust-lang#146505 (comment)) about a blanket lint that does not check the signature, which is now done with this PR, and about linting of `std` runtime symbols when std is not linked, which this PR omits by not including any std runtime symbols (for now).
## `invalid_runtime_symbol_definitions`
*(deny-by-default)*
The `invalid_runtime_symbol_definitions` lint checks the signature of items whose symbol name is a runtime symbols expected by `core`.
### Example
```rust,compile_fail
#[unsafe(no_mangle)]
pub fn memcmp() {} // invalid definition of the `memcmp` runtime symbol
```
```text
error: invalid definition of the runtime `memcmp` symbol used by the standard library
--> a.rs:2:1
|
4 | fn memcmp() {}
| ^^^^^^^^^^^
|
= note: expected `unsafe extern "C" fn(*const c_void, *const c_void, usize) -> i32`
found `fn()`
= help: either fix the signature or remove any attributes like `#[unsafe(no_mangle)]`, `#[unsafe(export_name = "memcmp")]`, or `#[link_name = "memcmp"]`
= note: `#[deny(invalid_runtime_symbol_definitions)]` on by default
```
### Explanation
Up-most care is required when defining runtime symbols assumed and used by the standard library. They must follow the C specification, not use any standard-library facility or undefined behavior may occur.
The symbols currently checked are `memcpy`, `memmove`, `memset`, `memcmp`, `bcmp` and `strlen`.
[^1]: https://doc.rust-lang.org/core/index.html#how-to-use-the-core-library
Disable `main_needs_argc_argv` for Wasm AFAIU this explains to the "Rust Runtime" that `main()` doesn't need `argc`/`argv`. Newer Wasm targets have explicitly disabled this, this PR changes it so that the base Wasm configuration affecting all Wasm targets disables this now. This affects the following targets: - `wasm32-unknown-unknown` - `wasm32v1-none` - `wasm64-unknown-unknown` The only Wasm target where `main_needs_argc_argv` is still enabled is `wasm32-unknown-emscripten`. @hoodmane let me know and I can remove it there as well. Credit goes to @Spxg for stumbling on this. r? @alexcrichton
Contributor
Author
|
@bors r+ rollup=never p=5 |
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
|
💔 Test for 77f0eec failed: CI. Failed job:
|
Contributor
|
PR #156571, which is a member of this rollup, was unapproved. |
Contributor
|
PR #155521, which is a member of this rollup, was unapproved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Successful merges:
main_needs_argc_argvfor Wasm #156571 (Disablemain_needs_argc_argvfor Wasm)r? @ghost
Create a similar rollup