Skip to content

Export #[inline] fns with extern indicators#73034

Merged
bors merged 7 commits intorust-lang:masterfrom
cofibrant:nomangle-inline-linkage
Jun 19, 2020
Merged

Export #[inline] fns with extern indicators#73034
bors merged 7 commits intorust-lang:masterfrom
cofibrant:nomangle-inline-linkage

Conversation

@cofibrant
Copy link
Contributor

@cofibrant cofibrant commented Jun 5, 2020

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
Copy link
Contributor

rust-highfive commented Jun 5, 2020

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2020
ollie27
ollie27 reviewed Jun 5, 2020
View reviewed changes
src/librustc_typeck/collect.rs Outdated
Copy link
Contributor

@ollie27 ollie27 Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cofibrant cofibrant Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/librustc_typeck/collect.rs Outdated
Copy link
Contributor

@ollie27 ollie27 Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@cofibrant cofibrant Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@Amanieu Amanieu Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cofibrant cofibrant requested review from Amanieu and ollie27 June 8, 2020 08:37
@cofibrant cofibrant changed the title Export #[inline] #[no_mangle] fns in cdylibs and staticlibs Export #[inline] fns with extern indicators Jun 8, 2020
matthewjasper
matthewjasper reviewed Jun 9, 2020
View reviewed changes
@matthewjasper
Copy link
Contributor

matthewjasper commented Jun 9, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 9, 2020

📌 Commit f57893b has been approved by matthewjasper

@bors 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 pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 11, 2020
@Dylan-DPC
Rollup merge of rust-lang#73034 - doctorn:nomangle-inline-linkage, r=…
f31414d 
…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)
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Jun 11, 2020
Closed
@Aaron1011
Copy link
Contributor

Aaron1011 commented Jun 11, 2020

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

@bors 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
@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Jun 11, 2020

failed in rollup log

@bors r-

@cofibrant
Copy link
Contributor Author

cofibrant commented Jun 15, 2020

@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
Copy link
Member

mati865 commented Jun 15, 2020

@DoctorN do you have sanitizers enabled?

rust/config.toml.example

Line 210 in ce6d3a7

#sanitizers = false

@cofibrant
Copy link
Contributor Author

cofibrant commented Jun 15, 2020 •
edited
Loading

@mati865 that'll do it 😅 Thanks!

@cofibrant
Copy link
Contributor Author

cofibrant commented Jun 15, 2020 •
edited
Loading

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?

@cofibrant
Copy link
Contributor Author

cofibrant commented Jun 15, 2020 •
edited
Loading

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

tmiasko
tmiasko reviewed Jun 15, 2020
View reviewed changes
@matthewjasper
Copy link
Contributor

matthewjasper commented Jun 15, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 15, 2020

📌 Commit e8e0a0e has been approved by matthewjasper

@bors 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 added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
@Manishearth
Rollup merge of rust-lang#73034 - doctorn:nomangle-inline-linkage, r=…
3bbc4fd 
…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)
@Manishearth Manishearth mentioned this pull request Jun 16, 2020
Closed
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 18, 2020
@Manishearth
Rollup merge of rust-lang#73034 - doctorn:nomangle-inline-linkage, r=…
89825e2 
…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)
@Manishearth Manishearth mentioned this pull request Jun 18, 2020
Closed
@Manishearth Manishearth mentioned this pull request Jun 18, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2020
@bors
Auto merge of rust-lang#73486 - Manishearth:rollup-11iyqpc, r=Manishe…
a39c778 
…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
@bors bors merged commit 0e332e9 into rust-lang:master Jun 19, 2020
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewjasper matthewjasper matthewjasper left review comments

@tmiasko tmiasko tmiasko left review comments

@Amanieu Amanieu Awaiting requested review from Amanieu

@ollie27 ollie27 Awaiting requested review from ollie27

Assignees

@matthewjasper matthewjasper

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.46

Development

Successfully merging this pull request may close these issues.

Warn if a function is marked as both #[inline] and #[no_mangle] #[inline] prevents symbol from appearing in staticlib output