Add lint against 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 |
|
@bors p=1 to get around tree closure |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 60c3805 failed: CI. Failed job:
|
|
@bors retry |
This comment has been minimized.
This comment has been minimized.
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test for 8e191f3 failed: CI. Failed job:
|
|
@bors retry |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for d7af889 failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 0f09409 failed: CI. Failed job:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Looks like it's new spurious failure. I'm seeing in on other PRs (#158115 (comment)), and of course it doesn't reproduce locally for me. Re-adding to the queue, hopefully it will pass next time 🤞 @bors r=davidtwco |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 390231d failed: CI. Failed job:
|
|
@bors retry |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for d12bbc8 failed: CI. Failed job:
|
|
@bors retry |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing eb6346c (parent) -> ce9954c (this PR) Test differencesShow 9 test diffsStage 1
Stage 2
Additionally, 6 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard ce9954c0cfc4bf26b82aef16e6fd8b020c237992 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (ce9954c): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.0%, secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 485.381s -> 486.597s (0.25%) |
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 bycorediffers significantly from the expected signature (like mismatch ABI, mismatch C variadics, mismatch argument count, missing return type, ...).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.suspicious_runtime_symbol_definitions(warn-by-default, asked in this comment)
The
suspicious_runtime_symbol_definitionslint checks the signature of items whose symbol name is a runtime symbol 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.
@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 ↩