Add lint againts invalid runtime symbol definitions

[Urgau]

This PR adds a deny-by-default lint againts invalid runtime symbol definitions, those runtime symbols are assumed and used by core¹ 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?
• Pointer becomes misaligned in test with no_mangle

This 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 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

#[unsafe(no_mangle)]
pub fn memcmp() {} // invalid definition of the `memcmp` runtime symbol

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.

@rustbot labels +I-lang-nominated +T-lang +needs-fcp +A-lints
cc @rust-lang/lang-ops
r? compiler

Footnotes

1. https://doc.rust-lang.org/core/index.html#how-to-use-the-core-library ↩

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[PatchMixolydic]

Nit:

Suggested change

	/// Up-most care is required when defining runtime symbols assumed and
	/// Utmost care is required when defining runtime symbols assumed and

View changes since the review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[traviscross]

Thanks @Urgau for adjusting based on the feedback and moving this forward.

@rfcbot fcp merge lang

[jackh726]

Should this also include rust_eh_personality?

[steffahn]

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 open and read) might also be desirable. All of this can of course be follow-up work so it shouldn’t block us from getting the lint as in this PR; but it’s be useful to clarify that the user reports (currently mentioned in the OP of this PR) aren’t addressed yet with this PR merged, and in this regard this isn’t a full replacement of #146505.

[scottmcm]

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 extern function declarations in rlibs, but if this expanded to a general check that you're not conflicting with other rust libraries that'd also be cool -- not this PR I assume, though.)

Another lint, for "it's very suspicious that you're defining an unmangled open" kinds of things probably also makes sense. That might not be deny though, since it's not as unquestionably wrong. It's a rare case of a rustc lint where I'm plausibly fine with the only mitigation being to allow it, since you could just crate-level-allow it for those very few crates where doing this is intentional because it's trying to implement such things. (Notably if you're trying to define a memcpy you'll need to do extra steps already anyway, IIRC.) But that wouldn't be this PR.

[traviscross]

(The bot has been fixed.)

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.