Export `#[inline]` fns with extern indicators #73034

doctorn · 2020-06-05T15:53:06Z

In ancient history (#36280) we stopped #[inline] fns being codegened if they weren't used. However,

observe that when writing something like

#![crate_type = "cdylib"]

#[no_mangle]
#[inline]
pub extern "C" fn foo() {
    // ...
}

we really do want foo to be codegened. This change makes this the case.

Resolves #72944, resolves #72463 (and maybe some more)

rust-highfive · 2020-06-05T15:53:09Z

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

ollie27 · 2020-06-05T22:41:19Z

src/librustc_typeck/collect.rs

+    // with "internal" linkage and are never exported unless we're
+    // building a `staticlib` or `cdylib` and they are marked
+    // `#[no_mangle]`.
+    tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NO_MANGLE)


There is also #[export_name = "..."] and #[linkage = "..."] to consider so this should really be calling contains_extern_indicator.

I wasn't aware of contains_extern_indicator, thank you so much for the suggestion!

ollie27 · 2020-06-05T22:48:21Z

src/librustc_typeck/collect.rs

+    // `#[no_mangle]`.
+    tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::NO_MANGLE)
+        && (tcx.sess.crate_types().contains(&CrateType::Cdylib)
+            || tcx.sess.crate_types().contains(&CrateType::Staticlib))


Do we need to check the crate type here? Even when building rlibs we'll still need to codegen and export #[no_mangle] #[inline] items in case they're linked into a staticlib or cdylib.

Just to check I understand between these two you're saying that #[inline] should always be superseded by an extern indicator and crate-type/specifics of indicator are irrelevant?

Yes, this should be done regardless of the output crate type.

src/librustc_middle/mir/mono.rs

matthewjasper · 2020-06-09T17:05:35Z

@bors r+

bors · 2020-06-09T17:05:37Z

📌 Commit f57893b has been approved by matthewjasper

…matthewjasper Export `#[inline]` fns with extern indicators In ancient history (rust-lang#36280) we stopped `#[inline]` fns being codegened if they weren't used. However, - rust-lang#72944 - rust-lang#72463 observe that when writing something like ```rust #![crate_type = "cdylib"] #[no_mangle] #[inline] pub extern "C" fn foo() { // ... } ``` we really _do_ want `foo` to be codegened. This change makes this the case. Resolves rust-lang#72944, resolves rust-lang#72463 (and maybe some more)

Aaron1011 · 2020-06-11T16:46:06Z

@bors r-

Failed in #73240 (comment): The function random_inline in test codegen/sanitizer-no-sanitize-inlining.rs no longer gets the fastcc calling convention assigned by LLVM, since it is now visible outside of the module.

Dylan-DPC-zz · 2020-06-11T16:54:56Z

failed in rollup log

@bors r-

doctorn · 2020-06-15T08:58:30Z

@bors r-

Failed in #73240 (comment): The function random_inline in test codegen/sanitizer-no-sanitize-inlining.rs no longer gets the fastcc calling convention assigned by LLVM, since it is now visible outside of the module.

I can't reproduce this locally, is this an architecture specific failure?

mati865 · 2020-06-15T09:07:48Z

@doctorn do you have sanitizers enabled?

rust/config.toml.example

Line 210 in ce6d3a7

#sanitizers = false

doctorn · 2020-06-15T09:08:17Z

@mati865 that'll do it 😅 Thanks!

doctorn · 2020-06-15T09:09:45Z

So looking into this, random_inline only needs to be no_mangle so that it can be mentioned in the test, ~~can we have a private #[no_export] attribute to deal with this kind of case?~~

doctorn · 2020-06-15T09:22:31Z

~~I've update the sanitizer test so that it doesn't depend on the #[inline] symbol meaning that it's ok to mangle it~~

src/test/codegen/sanitizer-no-sanitize-inlining.rs

matthewjasper · 2020-06-15T13:40:32Z

@bors r+

bors · 2020-06-15T13:40:33Z

📌 Commit e8e0a0e has been approved by matthewjasper

…matthewjasper Export `#[inline]` fns with extern indicators In ancient history (rust-lang#36280) we stopped `#[inline]` fns being codegened if they weren't used. However, - rust-lang#72944 - rust-lang#72463 observe that when writing something like ```rust #![crate_type = "cdylib"] #[no_mangle] #[inline] pub extern "C" fn foo() { // ... } ``` we really _do_ want `foo` to be codegened. This change makes this the case. Resolves rust-lang#72944, resolves rust-lang#72463 (and maybe some more)

@ghost

…arth Rollup of 17 pull requests Successful merges: - rust-lang#70551 (Make all uses of ty::Error delay a span bug) - rust-lang#71338 (Expand "recursive opaque type" diagnostic) - rust-lang#71976 (Improve diagnostics for `let x += 1`) - rust-lang#72279 (add raw_ref macros) - rust-lang#72628 (Add tests for 'impl Default for [T; N]') - rust-lang#72804 (Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position) - rust-lang#72814 (remove visit_terminator_kind from MIR visitor) - rust-lang#72836 (Complete the std::time documentation to warn about the inconsistencies between OS) - rust-lang#72968 (Only highlight doc search results via mouseover if mouse has moved) - rust-lang#73034 (Export `#[inline]` fns with extern indicators) - rust-lang#73315 (Clean up some weird command strings) - rust-lang#73320 (Make new type param suggestion more targetted) - rust-lang#73361 (Tweak "non-primitive cast" error) - rust-lang#73425 (Mention functions pointers in the documentation) - rust-lang#73428 (Fix typo in librustc_ast docs) - rust-lang#73447 (Improve document for `Result::as_deref(_mut)` methods) - rust-lang#73476 (Added tooltip for should_panic code examples) Failed merges: r? @ghost

rust-highfive assigned matthewjasper Jun 5, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2020

ollie27 reviewed Jun 5, 2020

View reviewed changes

doctorn requested review from Amanieu and ollie27 June 8, 2020 08:37

doctorn changed the title ~~Export #[inline] #[no_mangle] fns in cdylibs and staticlibs~~ Export #[inline] fns with extern indicators Jun 8, 2020

matthewjasper reviewed Jun 9, 2020

View reviewed changes

src/librustc_middle/mir/mono.rs Show resolved Hide resolved

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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 9, 2020

Dylan-DPC-zz mentioned this pull request Jun 11, 2020

Rollup of 8 pull requests #73240

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 11, 2020

doctorn added 5 commits June 15, 2020 09:40

Export #[inline] #[no_mangle] fns in cdylibs and staticlibs

f62903b

Export all fns with extern indicator

6b7cacb

Fix whitespace

d23bedd

Fix exports with #[inline(always)]

ee810a7

Fix whitespace

11b56fb

Fix sanitizer test

babda94

tmiasko reviewed Jun 15, 2020

View reviewed changes

src/test/codegen/sanitizer-no-sanitize-inlining.rs Outdated Show resolved Hide resolved

Update sanitizer test

e8e0a0e

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 Jun 15, 2020

Manishearth mentioned this pull request Jun 16, 2020

Rollup of 14 pull requests #73392

Closed

Manishearth mentioned this pull request Jun 18, 2020

Rollup of 17 pull requests #73475

Closed

Manishearth mentioned this pull request Jun 18, 2020

Rollup of 17 pull requests #73486

Merged

bors merged commit 0e332e9 into rust-lang:master Jun 19, 2020

cuviper added this to the 1.46 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export `#[inline]` fns with extern indicators #73034

Export `#[inline]` fns with extern indicators #73034

doctorn commented Jun 5, 2020

rust-highfive commented Jun 5, 2020

ollie27 Jun 5, 2020

doctorn Jun 5, 2020

ollie27 Jun 5, 2020

doctorn Jun 5, 2020

Amanieu Jun 6, 2020

matthewjasper commented Jun 9, 2020

bors commented Jun 9, 2020

Aaron1011 commented Jun 11, 2020

Dylan-DPC-zz commented Jun 11, 2020

doctorn commented Jun 15, 2020

mati865 commented Jun 15, 2020

doctorn commented Jun 15, 2020 •

edited

Loading

doctorn commented Jun 15, 2020 •

edited

Loading

doctorn commented Jun 15, 2020 •

edited

Loading

matthewjasper commented Jun 15, 2020

bors commented Jun 15, 2020

Export #[inline] fns with extern indicators #73034

Export #[inline] fns with extern indicators #73034

Conversation

doctorn commented Jun 5, 2020

rust-highfive commented Jun 5, 2020

ollie27 Jun 5, 2020

Choose a reason for hiding this comment

doctorn Jun 5, 2020

Choose a reason for hiding this comment

ollie27 Jun 5, 2020

Choose a reason for hiding this comment

doctorn Jun 5, 2020

Choose a reason for hiding this comment

Amanieu Jun 6, 2020

Choose a reason for hiding this comment

matthewjasper commented Jun 9, 2020

bors commented Jun 9, 2020

Aaron1011 commented Jun 11, 2020

Dylan-DPC-zz commented Jun 11, 2020

doctorn commented Jun 15, 2020

mati865 commented Jun 15, 2020

doctorn commented Jun 15, 2020 • edited Loading

doctorn commented Jun 15, 2020 • edited Loading

doctorn commented Jun 15, 2020 • edited Loading

matthewjasper commented Jun 15, 2020

bors commented Jun 15, 2020

Export `#[inline]` fns with extern indicators #73034

Export `#[inline]` fns with extern indicators #73034

doctorn commented Jun 15, 2020 •

edited

Loading

doctorn commented Jun 15, 2020 •

edited

Loading

doctorn commented Jun 15, 2020 •

edited

Loading