Issue an error when using `no_mangle` on language items #140203

alexandruradovici · 2025-04-23T14:43:02Z

This pull requests adds the code to issue an error or a warning when using no_mangle on language items. This should detail why the undefined symbol error is issued for the code described in #139923.

The pull request adds two ui tests, one testing the error and the other one the warning.

I would love some feedback here, as I am not sure that the error and warning are issues using the right API.

rustbot · 2025-04-23T14:43:08Z

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

alexandruradovici · 2025-04-23T14:43:53Z

r? @bjorn3

alexandruradovici · 2025-04-23T15:36:37Z

This fails due to a test that defines #[panic_handler] in a wrong place that also defines #[no_mangle].

#[panic_handler] //~ ERROR `panic_impl` lang item must be applied to a function
#[no_mangle]
static X: u32 = 42;

This somehow seems to be an error priority issue, the error that this pull request issues is correct.

I am not sure exactly how to avoid this, as I don't think checking that the target is a function is correct, as some language items are not functions.

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

Noratrieb · 2025-04-25T06:01:28Z

This fails due to a test that defines #[panic_handler] in a wrong place that also defines #[no_mangle].
#[panic_handler] //~ ERROR `panic_impl` lang item must be applied to a function
#[no_mangle]
static X: u32 = 42;
This somehow seems to be an error priority issue, the error that this pull request issues is correct.

I am not sure exactly how to avoid this, as I don't think checking that the target is a function is correct, as some language items are not functions.

you can probably just fix the test if you look at the history of the test and ensure that what it's trying to test is still tested

alexandruradovici · 2025-04-25T09:04:39Z

Thank you @Noratrieb

I deleted the no_mangle attribute from the ui/panic-handler/panic-handler-wrong-location. I am not sure why that was placed there, but as it now is an error, I think this should be correct. I might be wrong though, so any feedback is welcome.

My understanding is that the test should verify that the panic_handler attribute is used on a function. The no_mangle attribute should impact it in any way. The test still issues the same error, panic_impl lang item must be applied to a function, when the no_mangle is deleted.

rustbot · 2025-04-28T08:32:28Z

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

bjorn3 · 2025-04-28T10:18:12Z

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

        if WEAK_LANG_ITEMS.contains(&lang_item) {
            codegen_fn_attrs.flags |= CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL;
+            if has_no_mangle {


This code is only running when you are defining a lang item. I think it should prevent all combinations of #[rustc_std_internal_symbol] and #[no_mangle]. Can you move it below the lang_items::extract if?

Just to make sure I understand this correctly, you're suggesting to add another if after the lang_items::extract:

if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_MANGLE) { if is_weak_symbol { // issue error } else if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::RUSTC_STD_INTERNAL_SYMBOL) { // issue warning } }

Looking at it a bit more careful, I think keeping the error in the same location would be fine. The warning should be moved out of the lang item if however.

Also I noticed that you didn't make the warning a future incompat warning: https://rustc-dev-guide.rust-lang.org/diagnostics.html#future-incompatible-lints

Looking at it a bit more careful, I think keeping the error in the same location would be fine. The warning should be moved out of the lang item if however.

Got it.

Also I noticed that you didn't make the warning a future incompat warning: https://rustc-dev-guide.rust-lang.org/diagnostics.html#future-incompatible-lints

Yep, I wasn't sure if this was the indented way, I'll change it. I have two questions here:

what issue should I reference?

what should be the reason? FutureReleaseErrorDontReportInDeps?

FutureReleaseErrorReportInDeps should be fine as reason. We don't need to emit a future incompat error for long given that #[rustc_std_internal_symbol] is unstable. As for the issue, that would be a newly created tracking issue for this future incompat warning.

On the other hand, the nightly versions already issue linking errors.

Yeah. I think emitting an error immediately for this case is fine.

Do you have an example of a language item that would emit a warning? I am not able to find one that would fit the criteria and I want to have a test for it.

I think the only lang items for which #[no_mangle] is actively harmful are those that are weak lang items or those that are generic. Weak lang items would be a hard error and generic lang items don't allow #[no_mangle] already I think. Maybe just omit the warning case?

I think I agree, as it turns out really difficult to get a case where the warning is issued.

I modified the code to issue just the error.

tests/ui/codegen/no-mangle-on-internal-lang-items.rs

bjorn3 · 2025-04-30T09:04:22Z

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

+            )
+            .with_note("The linker requires specific names for internal language items.")
+            .with_note("If you are trying to prevent mangling to ease debugging, many")
+            .with_note("debuggers support a command such as `rbreak {sym}` to")


You need to use format!() for the {sym} to be replaced by the value of sym.

I don't think @ppannuto purpose was rot replace these. In any case, we would need the mangled name. Is there any way to get it, as I am assuming that mangling is performed later on.

I used the local name to replace sym as anyway this would be a regular expression match, but it is the name before the #[rustc_std_internal_symbol]

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

bjorn3 · 2025-04-30T09:06:51Z

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

+        let label = if let Some(lang_item) = lang_item {
+            format!("should be the {} item", lang_item.name())
+        } else {
+            format!("should be the internal language item")
+        };


I don't think this label is useful.

I wanted a way to show the user the problematic item, not just the attribute.

bjorn3 · 2025-04-30T09:09:03Z

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

+            if let Some(span) = span {
+                let label_attr = format!("this defines it as the `{}` item", lang_item.name());
+                err = err.with_span_label(span, label_attr);
+            }


This leaks the internal lang item name for #[panic_handler].

Is there any other way to say what language item this is? I have not seen any way of saying which language item it is.

For #[panic_handler] we should say anything about lang items at all (it would be leaking an internal implementation detail) and for the rest I'm not sure it is all that useful anyway.

I deleted it for now.

bjorn3 · 2025-04-30T09:09:58Z

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

+            if let Some(link_name) = lang_item.link_name() {
+                let renamed =
+                    format!("In this case it is automatically renamed to `{}`", link_name);
+                err = err.with_note(renamed);
+            }


This is showing the name before #[rustc_std_internal_symbol] mangling.

Is there any way to get the mangled name? I am assuming no, as the mangling is done later on.

I tried this:

let instance = Instance::mono(tcx, did.into()); let name = tcx.symbol_name(instance).name;

but it throws a cycle detected error.

bjorn3

r=me with the commented out code removed.

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

bjorn3 · 2025-04-30T11:37:32Z

Would you mind squashing your changes?

alexandruradovici · 2025-04-30T11:52:53Z

Would you mind squashing your changes?

Done

add suggestion on how to add a panic breakpoint Co-authored-by: Pat Pannuto <pat.pannuto@gmail.com> delete no_mangle from ui/panic-handler/panic-handler-wrong-location test issue an error for the usage of #[no_mangle] on internal language items delete the comments add newline rephrase note Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com> update error not to leak implementation details delete no_mangle_span Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com> delete commented code

bjorn3 · 2025-04-30T12:11:16Z

@bors r+

bors · 2025-04-30T12:11:19Z

📌 Commit 07c7e5f has been approved by bjorn3

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#134232 (Share the naked asm impl between cg_ssa and cg_clif) - rust-lang#139624 (Don't allow flattened format_args in const.) - rust-lang#140090 (Check bare function idents for non snake-case name) - rust-lang#140203 (Issue an error when using `no_mangle` on language items) - rust-lang#140450 (ast: Remove token visiting from AST visitor) - rust-lang#140498 (Misc tweaks to HIR typeck (mostly w.r.t. checking calls)) - rust-lang#140504 (transmutability: ensure_sufficient_stack when answering query) - rust-lang#140506 (unstable-book: fix capitalization) - rust-lang#140516 (Replace use of rustc_type_ir by rustc_middle) Failed merges: - rust-lang#140374 (Resolve instance for SymFn in global/naked asm) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#140203 - Wyliodrin:error_for_no_mangle_weak_language_items, r=bjorn3 Issue an error when using `no_mangle` on language items This pull requests adds the code to issue an error or a warning when using `no_mangle` on language items. This should detail why the `undefined symbol` error is issued for the code described in rust-lang#139923. The pull request adds two ui tests, one testing the error and the other one the warning. I would love some feedback here, as I am not sure that the error and warning are issues using the right API.

rustbot assigned petrochenkov Apr 23, 2025

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 23, 2025

rustbot assigned bjorn3 and unassigned petrochenkov Apr 23, 2025

alexandruradovici mentioned this pull request Apr 23, 2025

undefined symbol: __rustc::rust_begin_unwind when using panic="abort" #139923

Open

alexandruradovici force-pushed the error_for_no_mangle_weak_language_items branch from f599390 to caa2b04 Compare April 23, 2025 14:58

This comment has been minimized.

Sign in to view

ppannuto reviewed Apr 23, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/codegen_attrs.rs Outdated Show resolved Hide resolved

alexandruradovici marked this pull request as ready for review April 28, 2025 08:32

bjorn3 reviewed Apr 28, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

bjorn3 reviewed Apr 30, 2025

View reviewed changes

tests/ui/codegen/no-mangle-on-internal-lang-items.rs Outdated Show resolved Hide resolved

bjorn3 requested changes Apr 30, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2025

alexandruradovici changed the title ~~Issue an error or warning when using no_mangle on language items~~ Issue an error when using no_mangle on language items Apr 30, 2025

This comment has been minimized.

Sign in to view

bjorn3 approved these changes Apr 30, 2025

View reviewed changes

compiler/rustc_codegen_ssa/src/codegen_attrs.rs Outdated Show resolved Hide resolved

alexandruradovici force-pushed the error_for_no_mangle_weak_language_items branch from 43114d3 to 51a0bf0 Compare April 30, 2025 11:52

alexandruradovici force-pushed the error_for_no_mangle_weak_language_items branch from 51a0bf0 to 07c7e5f Compare April 30, 2025 11:54

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2025

matthiaskrgr mentioned this pull request Apr 30, 2025

Rollup of 9 pull requests #140520

Merged

bors merged commit 1a64f2c into rust-lang:master Apr 30, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone Apr 30, 2025

Issue an error when using no_mangle on language items #140203

Issue an error when using no_mangle on language items #140203

Conversation

alexandruradovici commented Apr 23, 2025 • edited Loading

rustbot commented Apr 23, 2025

alexandruradovici commented Apr 23, 2025

This comment has been minimized.

alexandruradovici commented Apr 23, 2025 • edited Loading

Noratrieb commented Apr 25, 2025

alexandruradovici commented Apr 25, 2025 • edited Loading

rustbot commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

bjorn3 left a comment

Choose a reason for hiding this comment

bjorn3 commented Apr 30, 2025

alexandruradovici commented Apr 30, 2025

bjorn3 commented Apr 30, 2025

bors commented Apr 30, 2025

Issue an error when using `no_mangle` on language items #140203

Issue an error when using `no_mangle` on language items #140203

alexandruradovici commented Apr 23, 2025 •

edited

Loading

alexandruradovici commented Apr 23, 2025 •

edited

Loading

alexandruradovici commented Apr 25, 2025 •

edited

Loading

bjorn3 Apr 30, 2025 •

edited

Loading