Disallow export_name starting with "llvm."

[moxian]

Fixes #140822
Motivation in the issue, and in code comments.

This implementation adds the check to right to #[export_name] parsing code. I considered moving it closer to the place where the new name gets emitted to llvm, but that looked harder to get right, and easier to accidentally break going forward.

I don't believe any other attributes need this handling. The only one that comes close is link_name but that one uses the name, and does not define it, and in fact #[link_name = "llvm.stuff"] is entirely justified and actively used in real code.

Two small drive-by changes in separate commits (removed an obsolete and misleading //FIXME comment, and tighten the diagnostics span for the null-byte-in-export_name check). Happy to split them into dedicated PR(s) if this is deemed valuable.

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
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

[rustbot]

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

cc @jdonszelmann

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[jieyouxu]

Note

Please ignore this nomination, as it's probably more of a bug-fix. Left unresolved for reference only.

Background

Rejecting #[export_name] whose value starts with llvm. seems very reasonable, but then it also feels at the same time somewhat arbitrary, because we now hard-error on "certain" #[export_name] values (namely, those that begin with llvm.). A few considerations:

1. This was rejected / permitted in an oscillating fashion previously, as the rejection depended on verify-llvm-ir (i.e. the rust frontend was not the one "actively" (or rather, explicitly) performing the rejection).

2. We almost certainly need to be rejecting this, because otherwise

   Creating a definition for an intrinsic is invalid LLVM IR, the rest is not really relevant. The Rust frontend must generate an error when trying to use an export_name starting with llvm..
   -- Target features getting erased when using #[export_name] with names of LLVM intrinsics #140822 (comment)

3. We should make an explicit decision whether the rustc frontend should be explicitly rejecting this.

Possible breakage

I don't really expect any breakages (someone is likely doing some... weird things if this causes their code to no longer compile), but this is technically constraining the set of programs that we accept.

Nomination questions Some questions

• (T-compiler, T-lang) Should we perform this explicit rejection for export_names that begin with llvm.?
• (T-compiler, T-lang) Does the llvm. rejection also happen when the cg backend is e.g. cg_gcc? Or do we only reject llvm. when cg backend is cg_llvm?
  • Probably universally reject (across any and all cg backends, that is)?
• (T-lang) If we universally reject, do we document this at the language level (Reference) as sth like "IR intrinsic name limitations"?

stale

[jieyouxu]

EDIT: I removed the lang nomination, probably more of a "bug fix" / "impl detail".

[Noratrieb]

GitHub code search yields no results; https://github.com/search?q=%22export_name+%3D+%5C%22llvm.%22&type=code
(it finds stuff without the period so it's probably not wrong: https://github.com/search?q=%22export_name+%3D+%5C%22llvm%22&type=code)

this is such a weird and broken thing to do that I think we can just ship this without crater or FCW or anything

[lcnr]

r? compiler

[WaffleLapkin]

Is there a reason this is handled in rustc_codegen_ssa and not rustc_codegen_llvm? I assume while weird, #[export_name = "llvm...."] should not break any other backends.

[Urgau]

Is there a reason this is handled in rustc_codegen_ssa and not rustc_codegen_llvm? I assume while weird, #[export_name = "llvm...."] should not break any other backends.

Wouldn't it be weird if your code is working on one backend but not on another one?

[WaffleLapkin]

@Urgau generally yes, but

1. this might already happen
2. the name overlaps with the backend's internal symbol, so I don't think in this case it's weird

[jieyouxu]

@Urgau generally yes, but

1. this might already happen

2. the name overlaps with the backend's internal symbol, so I don't think in this case it's weird

Yeah, I should've left the nomination comment open, also asked

• (T-compiler, T-lang) Should we perform this explicit rejection for export_names that begin with llvm.?
• (T-compiler, T-lang) Does the llvm. rejection also happen when the cg backend is e.g. cg_gcc? Or do we only reject llvm. when cg backend is cg_llvm?
  • Probably universally reject (across any and all cg backends, that is)?
• (T-lang) If we universally reject, do we document this at the language level (Reference) as sth like "IR intrinsic name limitations"?

[traviscross]

• If we universally reject, do we document this at the language level (Reference) as sth like "IR intrinsic name limitations"?

Yes, this should be documented in the Reference.

@rustbot labels +S-waiting-on-documentation

cc @ehuss

[WaffleLapkin]

I really don't think a singular codegen backend's limitation should be part of the language. I don't think we'd do something like this for other backends and I think llvm shouldn't be special in this regard.

[nikic]

I really don't think a singular codegen backend's limitation should be part of the language. I don't think we'd do something like this for other backends and I think llvm shouldn't be special in this regard.

It's possible to support this for the LLVM backend by emitting a mangling escape. But actually doing that would be incoherent with #[link_name], which does give llvm. special treatment (behind the link_llvm_intrinsics gate, which is active independent of backend I believe).

[nikic]

It's probably also worth pointing out that other backends actually do tend to support #[link_name="llvm.foo"] for some subset of LLVM intrinsics, because this is used extensively by stdarch.

So while this is certainly an LLVM-ism originally, the fact that llvm.* names are special is no longer limited to solely the LLVM backend.

[moxian]

Is there a reason this is handled in rustc_codegen_ssa and not rustc_codegen_llvm? I assume while weird, #[export_name = "llvm...."] should not break any other backends.

The actual reason was that I thought the implementation to be a bit easier and look a bit nicer this way.
But this is not a principled choice, and I'm happy to move the check to rustc_codegen_llvm if there is a consensus that that's preferable (but I'm not sure we have a consensus yet)

I really don't think a singular codegen backend's limitation should be part of the language.

My opinion doesn't have much weight here, but as a user I agree that this should not be part of the Language reference. It is only an implementation detail of rustc compiler.

I also considered making this a deny-by-default lint instead of hard error, but couldn't think of a good reason why would someone want to bypass this check so it's a hard error currently.

[moxian]

I'm happy to move the check to rustc_codegen_llvm if there is a consensus

On the second thought, that's an unfair framing by me. This check should go to rustc_codegen_llvm by default as a noncontroversial bugfix, restoring the behaviour we've had up until 1.84.0. The consensus is only neccessary to move it out of rustc_codegen_llvm into a more general rustc_codegen_ssa since that is a potentially breaking change (maybe not in practice, but in principle).

I'll try to move it to rustc_codegen_llvm, unless I get preempted.

@rustbot author

[Nadrieril]

r? codegen

[sayantn]

Are there similar "restricted" names for cg_gcc or cg_clif? I believe names starting with __builtin_ are reserved in gcc, so that should be handled somewhere too (maybe in a different pr)