Target features getting erased when using `#[export_name]` with names of LLVM intrinsics

[sayantn]

TIL Rust "supports" #[export_name] with names of LLVM intrinsics! The following code compiles no problem

#[unsafe(export_name = "llvm.x86.rdtsc")]
fn foo() {
    // ...
}

(FYI llvm.x86.rdtsc is a very real LLVM intrinsic, used to read the timestamp counter value in x86)
which imo should be hard error anyway. But then if we combine this monstrosity with #[target_feature] (or -C target-feature for that matter), all the target feature data for that function is erased. This is possible even in stable Rust, both in debug and release profile.

Godbolt link https://godbolt.org/z/h7jdrsY54

Meta

Doing a naive bisect on Godbolt, I found out that this bug was introduced in 1.29.0!!
https://godbolt.org/z/8M9a4vs9Y

The solution is probably just disallow any #[export_name]'s that try to masquerade as LLVM intrinsics (i.e. name starts with llvm.), which will mean reverting to pre-1.29.0 behavior.

@rustbot label T-compiler

[moxian]

The example on godbolt has been producing llvm IR, but refused to codegen between 1.29.0 and 1.84.0. Specifically, up until #133499 . So arguably there has been no "real bug" in that period.

The earlier change in 1.28.0 -> 1.29.0 (error -> no error in --emit=llvm-ir) points to either #51230 or (less likely) #51966

(full list of PRs in nightly-2018-07-13)

  - #51612 (NLL: fix E0594 "change to mutable ref" suggestion) by ashtneoi
  - #52229 (Rollup of 7 pull requests) by GuillaumeGomez
    - #51612 (NLL: fix E0594 "change to mutable ref" suggestion)
    - #51722 (Updated RELEASES for 1.28.0)
    - #52064 (Clarifying how the alignment of the struct works)
    - #52149 (Add #[repr(transparent)] to Atomic* types)
    - #52151 (Trait impl settings)
    - #52171 (Correct some codegen stats counter inconsistencies)
    - #52195 (rustc: Avoid /tmp/ in graphviz writing)
  - #52232 ( use the adjusted type for cat_pattern in tuple patterns) by arielb1
  - #51966 (Upgrade to LLVM's master branch (LLVM 7)) by alexcrichton
  - #52245 (Rollup of 5 pull requests) by GuillaumeGomez
    - #51701 (Better docs for copy_from_slice & clone_from_slice)
    - #52231 (Fix typo in error message E0277)
    - #52233 (Improve lint handling in rustdoc)
    - #52238 (Avoid unwrapping in PanicInfo doc example.)
    - #52241 (Fix typo in E0433 docs)
  - #51230 (Disable LLVM verification by default) by nikic
  - #51553 (Unix sockets on redox) by jD91mZM2
  - #51702 (Infinite loop detection for const evaluation) by ecstatic-morse
  - #52268 (Rollup of 14 pull requests) by Mark-Simulacrum
    - #51614 (Correct suggestion for println)
    - #51952 ( hygiene: Decouple transparencies from expansion IDs)
    - #52193 (step_by: leave time of item skip unspecified)
    - #52207 (improve error message shown for unsafe operations)
    - #52223 (Deny bare trait objects in in src/liballoc)
    - #52224 (Deny bare trait objects in in src/libsyntax)
    - #52239 (Remove sync::Once::call_once 'static bound)
    - #52247 (Deny bare trait objects in in src/librustc)
    - #52248 (Deny bare trait objects in in src/librustc_allocator)
    - #52252 (Deny bare trait objects in in src/librustc_codegen_llvm)
    - #52253 (Deny bare trait objects in in src/librustc_data_structures)
    - #52254 (Deny bare trait objects in in src/librustc_metadata)
    - #52261 (Deny bare trait objects in in src/libpanic_unwind)
    - #52265 (Deny bare trait objects in in src/librustc_codegen_utils)
  - #52172 (Inject clippy into the rls again) by oli-obk
  - #52089 (rustc_codegen_llvm: replace the first argument early in FnType::new_vtable.) by eddyb
  - #52194 (Remove rustdoc's plugins feature) by steveklabnik
  - #52282 (Patch clippy_lints to use the in-tree clippy as well.) by kennytm
  - #52230 (rustc: Search all derives for inert attributes) by alexcrichton
  - #52303 (Rollup of 9 pull requests) by kennytm
    - #51816 (bootstrap: write texts to a .tmp file first for atomicity)
    - #51912 (impl Clone for Box<CStr>, Box<OsStr>, Box<Path>)
    - #52164 (use proper footnote syntax for references)
    - #52220 (Deny bare trait objects in `src/bootstrap`)
    - #52276 (rustc: Verify #[proc_macro] is only a word)
    - #52277 (Uncapitalize "If")
    - #52287 (Deny bare trait objects in src/librustc_resolve)
    - #52295 (Deny bare trait objects in src/libsyntax_ext)
    - #52298 (make reference to dirs crate clickable in terminals)
  - #52256 (make pretty source comparison check be fatal (fixes #52255)) by tinco
  - #51339 (Add ExactChunks::remainder and ExactChunks::into_remainder) by sdroege

so this was never been a rustc native behaviour, and we've always relied on LLVM to catch this (until recently).

cc @nikic

I'm not quite sure how #[target_feature] is relevant here. The only thing it is seemingly doing in the godbolt example is specifying "target-features"=" in the attributes list (which is expected), but nothing else. Unless I'm badly misreading this https://godbolt.org/z/83M6noTG7

@rustbot label: +A-llvm

[sayantn]

If you look closely, you can see that the function exported with the existing llvm intrinsic name has different target features than the other ones (in your example, the llvm.x86.rdtsc one has #0, which doesn't have any target features, whereas the other 2 has #1, which has target feature avx).

Without the target feature thing, this isn't formally a "bug", just surprising (but predictable) behavior. When the target features get erased, it will start to throw rustc-LLVM errors because the inside block uses avx things, but now apparently avx is not available anymore (which is what target_feature protects us against)

[nikic]

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