Add lint againts invalid runtime symbol definitions#155521
Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| /// | ||
| /// ### Explanation | ||
| /// | ||
| /// Up-most care is required when defining runtime symbols assumed and |
There was a problem hiding this comment.
Nit:
| /// Up-most care is required when defining runtime symbols assumed and | |
| /// Utmost care is required when defining runtime symbols assumed and |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
Should this also include |
|
As far as I understood the lang meeting discussion, there was no concern with this PR. But some opinion that it doesn’t fully replace the previous PR, since some warn-by-default lint more generally applicable (and to actually address the user reports, which were about |
|
My interpretation here: this lint is valuable because the wrong signature for something on the well-known list is clearly and always wrong, so deny makes sense. We should have this lint. (And I'm find expanding to more functions so long as they meet that "definitely incontrovertibly wrong signature for that name that rustc or one of your linked libraries uses" bar. I don't know if we include Another lint, for "it's very suspicious that you're defining an unmangled |
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
|
This pull request was unapproved. This PR was contained in a rollup (#156595), which was unapproved. |
|
@bors try jobs=x86_64-gnu-llvm-21-3 |
This comment has been minimized.
This comment has been minimized.
Add lint againts invalid runtime symbol definitions try-job: x86_64-gnu-llvm-21-3
|
@bors r=davidtwco |
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
Rollup of 11 pull requests Successful merges: - #148788 (Unconstrained parameter fix) - #153238 (debuginfo: slices are DW_TAG_array_type's) - #155521 (Add lint againts invalid runtime symbol definitions) - #156319 (Require EIIs to be defined when we compile a rust dylib) - #156452 (Implement pinned drop sugar) - #156600 (Make const param default test reproduce original ICE) - #156493 (actually run the temp_dir doctest) - #156556 (Require UTF-8 in `Utf8Pattern::StringPattern`) - #156565 (delegation: emit error when self type is not specified and accessed) - #156586 (Use DropCtxt::new_block and new_block_with_statements systematically.) - #156587 (Correctly handle associated items in rustdoc macro expansion)
|
Failed again in #156610 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#156610), which was unapproved. |
|
@bors rollup=never (for next time) |
|
@rustbot ping rust-for-linux Seems like Is the mismatch between |
|
Hey Rust for Linux group! It looks like something broke the Rust for Linux integration. cc @rust-lang/rust-for-linux |
|
It is intended, because kernel C code is compiled with |
That doesn't really make sense to me, if the C code is compiled with a flag that change the sign of
Not easily, it's using a I could probably do some type-system foolery that check against modified function signature that replaces |
View all comments
This PR adds a deny-by-default lint againts invalid runtime symbol definitions, those runtime symbols are assumed and used by
core1 andrustcwith a specific definition.We have had multiple reports of users tripping over
stdsymbols (addressed in a future PR):#[no_mangle] fn open() {}makecargo thang?no_mangleThis PR is a second attempt after #146505, where T-lang had some reservations about a blanket lint that does not check the signature, which is now done with this PR, and about linting of
stdruntime 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_definitionslint checks the signature of items whose symbol name is a runtime symbols expected bycore.Example
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,bcmpandstrlen.@rustbot labels +I-lang-nominated +T-lang +needs-fcp +A-lints
cc @rust-lang/lang-ops
r? compiler
Footnotes
https://doc.rust-lang.org/core/index.html#how-to-use-the-core-library ↩