Add lint about redefining runtime symbols

[Urgau]

This PR adds lint to warn about redefinition of runtime symbols¹ that are assumed and used by core² and std.

We have had multiple reports of users tripping over this:

• Why does #[no_mangle] fn open() {} make cargo t hang?
• Pointer becomes misaligned in test with no_mangle

redefining_runtime_symbols

Old proposed name: clashing_function_names_with_fundamental_functions

(warn-by-default)

The redefining_runtime_symbols lint checks for items whose symbol name redefines a runtime symbols expected by core and/or std.

Example

#[unsafe(no_mangle)]
pub fn strlen() {} // redefines the libc `strlen` function

warning: redefinition of the runtime `strlen` symbol used by the standard library
 --> a.rs:2:1
  |
2 | pub fn strlen() {}
  | ^^^^^^^^^^^^^^^^^^
  |
  = note: extra care must be taken when redefining those symbols, they must match exactly (ABI, function arguments, function return type, behavior, ...)
  = note: see <https://doc.rust-lang.org/core/index.html#how-to-use-the-core-library> for the more details
  = help: either allow this lint or remove any `#[unsafe(no_mangle)]` or `#[unsafe(export_name = "strlen")]`
  = note: `#[warn(redefining_runtime_symbols)]` on by default

Explanation

Up-most care is required when redefining 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 respectively:

• from core²: memcpy, memmove, memset, memcmp, bcmp, strlen
• from std: open/open64, read, write, close

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

Footnotes

1. previous lint name clashing_function_names_with_fundamental_functions, bike-shed at https://github.com/rust-lang/rust/pull/146505#issuecomment-3288716835 ↩

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

[Urgau]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: fb73ebd (fb73ebde4cd6ae9fb27bac8017eab9dc519131f9, parent: 064cc81354a940e297a1be4dfa9e26759c8431be)

[rust-timer]

Finished benchmarking commit (fb73ebd): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary -0.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	8.2%	[8.2%, 8.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-2.5%, -1.1%]	6
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -2.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-3.6%, -1.6%]	2
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 468.794s -> 471.13s (0.50%)
Artifact size: 388.08 MiB -> 388.08 MiB (0.00%)

[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.

[traviscross]

To bikeshed the name:

I wouldn't call this "clashing" unless the lint is checking specifically that the signature doesn't match what is expected for each symbol. We have an existing clashing_extern_declarations lint, and that one does check for mismatched signatures. Maybe "redefined/redefining", "shadowed/shadowing", "colliding", etc. would work.

I wouldn't refer to "function names" here because a "function name" in Rust refers to the name of the function in Rust rather than to the symbol name, and we want to focus on the symbol name here.

Also, shouldn't we be checking for more than just functions? This breaks things too:

#[unsafe(no_mangle)]
static read: () = ();

I probably wouldn't call these functions "fundamental". Maybe "runtime", "system", "platform", "builtin", "libc", or similar would work.

Perhaps redefining_runtime_symbols (or maybe redefined_runtime_symbols) would be a decent name?

[Urgau]

Perhaps redefining_runtime_symbols (or maybe redefined_runtime_symbols) would be a decent name?

redefining_runtime_symbols works for me. Changed the lint name as such.

Also, shouldn't we be checking for more than just functions?

Indeed, forgot that statics also have a symbol. Fixed.

---

Regarding the lint level, I made it warn-by-default since there are legitimate reasons to implement those symbols (like when implementing a libc), but maybe it should be deny-by-default?

[bjorn3]

This could just be

        if let Some(name) = attrs.symbol_name {
            // Use provided name
            return name.to_string();
        }

        if attrs.flags.contains(CodegenFnAttrFlags::NO_MANGLE) {
            // Don't mangle
            return tcx.item_name(def_id).to_string();
        }

right? The rest of symbol_name_without_mangling is not relevant here.

[Urgau]

Technically yes, but I don't want the logic to be at two different places if we don't have to, and since the perf run is clean as is, I would like to keep symbol_name_without_mangling (to avoid duplicating the logic).

[bjorn3]

The function name is not entirely correct given that aside from this snippet, all code inside this function handles cases where there actually is symbol mangling because of #[rustc_std_internal_symbol], because of #[wasm_import_module] or because it is the static containing all defined proc macros.

[Urgau]

Yeah, the function name not great. It's should be more something like symbol_name_from_attrs, not sure what the right name should be.

EDIT: done

[tgross35]

What is the significance of documenting a split between core and std? It looks like they get checked the same.

[Urgau]

It's not very important; it's mainly a way to distinguish between explicitly documented symbols (those in core) and those that are more implicit (those in std).

Regarding the checking, it could be asked to not check the std runtime symbols in #![no_std], which I don't think we should do, #![no_std] crates may still end-up in std-full crates.