Lint on combining #[no_mangle] and #[export_name]

[sassman]

This is my very first contribution to the compiler, even though I read the chapter about lints I'm not very certain that this new lint is done right as a builtin lint PR is right. I appreciate any guidance on how to improve the code.

• Add test for issue Using both no_mangle and export_name attributes should cause a warning #47446
• Implement the new lint mixed_export_name_and_no_mangle as a builtin lint (not sure if that is the right way to go) Extend unused_attributes lint
• Add suggestion how to fix it

Old proposed new lint

The mixed_export_name_and_no_mangle lint detects usage of both #[export_name] and #[no_mangle] on the same item which results on #[no_mangle] being ignored.

warn-by-default

Example

#[no_mangle] // ignored
#[export_name = "foo"] // takes precedences
pub fn bar() {}

Explanation

The compiler will not respect the #[no_mangle] attribute when generating the symbol name for the function, as the #[export_name] attribute takes precedence. This can lead to confusion and is unnecessary.

Fixes #47446

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @TaKO8Ki (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[workingjubilee]

@Urgau Do you know why all these lints are prefixed by Builtin? Aren't all the lints in the compiler built-in by definition?

[Urgau]

Do you know why all these lints are prefixed by Builtin? Aren't all the lints in the compiler built-in by definition?

It's probably a prefix for the lints defined inside compiler/rustc_lint/src/builtin.rs.

Lints outside of that file don't seems to have it at least.

[workingjubilee]

Ah, I see.

Mm, great, in tests/ui/asm/naked-functions.rs we find this gem:

#[export_name = "exported_function_name"]
#[link_section = ".custom_section"]
#[no_mangle]
#[naked]
pub unsafe extern "C" fn compatible_ffi_attributes_1() {
    naked_asm!("", options(raw));
}

@sassman Thank you for PRing this lint! It looks like you will have to fix a few cases of it in our test suite as well, and --bless. Feel free to apply either, as you prefer, to relevant cases like that "naked" function.

[sassman]

Now the tests run all green and I have cleaned up the other test that used both no_mangle and export_name and simply removed the no_mangle in tests/ui/asm/naked-functions.rs.

Please let me know if there is anything that I can do further.

[Urgau]

I think this check is miss-placed, I think it should be with the others codegen attributes handling in the codegen_fn_attrs function in compiler/rustc_codegen_ssa/src/codegen_attrs.rs.

You can look at the check_link_name_xor_ordinal function for inspiration:

rust/compiler/rustc_codegen_ssa/src/codegen_attrs.rs

Line 648 in f6648f2

check_link_name_xor_ordinal(tcx, &codegen_fn_attrs, link_ordinal_span);

As well as TyCtxt::emit_node_span_lint for emitting the lint.
You will also need to move the lint definition into the rustc_lint_defs crate and import that crate in rustc_codegen_ssa.

@rustbot author

[sassman]

Thanks @Urgau for the guidance, I will refactor the code accordingly.
The provided example seems pretty helpful to figure things out.

[sassman]

@Urgau I have refactored the code, but just to be sure I get things right, with this approach we won't have "named" lints that a user can allow or deny anymore or am I missing something?

In the previous commit the lint was tied to the LintDiagnostic struct, that is now not anymore present.

If I did got this right then I will adjust the introduced error output expectation to fix the build error:
https://github.com/rust-lang/rust/actions/runs/11332684748/job/31515339659?pr=131558#step:26:3779

[Urgau]

Reminder, don't forget to use the rustbot review commands.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[sassman]

@rustbot review

[Urgau]

@bors r+

[bors]

📌 Commit 7951d19 has been approved by Urgau

It is now in the queue for this repository.

[Kobzol]

@rust-timer build 79254c6

[rust-timer]

Finished benchmarking commit (79254c6): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to 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 (primary -3.0%, secondary -0.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-3.0%	[-4.7%, -1.3%]	2
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-3.0%	[-4.7%, -1.3%]	2

Cycles

Results (secondary 2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[2.0%, 2.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results (secondary -0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	-	-	0

Bootstrap: 767.346s -> 766.133s (-0.16%)
Artifact size: 330.96 MiB -> 330.98 MiB (0.00%)