Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improper_ctypes: extern "C" fns #65134

Merged

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Oct 5, 2019

cc #19834. Fixes #65867.

This pull request implements the change described in this comment.

cc @rkruppe @varkor @shepmaster

@rust-highfive
Copy link
Collaborator

r? @cramertj

(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 Oct 5, 2019
@rust-highfive

This comment has been minimized.

@davidtwco davidtwco force-pushed the issue-19834-improper-ctypes-in-extern-C-fn branch from 6bc5976 to c5f18a7 Compare October 5, 2019 18:40
Copy link
Contributor

@hanna-kruppe hanna-kruppe left a comment

Choose a reason for hiding this comment

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

Inline comments/questions about some of the allows. The number of in-tree false positives, especially due to generics, is a bit concerning but seems surmountable. I can think of a few ways to address it, in increasing order of complexity and pay-off:

  • First of all, if possible, don't bail out on lifetime parameters. I've not given the code a careful reading w.r.t. this but it seems easier to not crash on those, and it would eliminate some false positives.
  • Consider not trying to lint (type- and const-)generic functions. As they can't be no_mangle anyway, the only way they'll leak to C is if they get passed as function pointer, and we do check the types occurring in function pointers. (There could still be casts, e.g. to void *.)
  • Check generic functions at monomorphization time. Errors at mono-time are a no-no but warnings should be fine. The downside is that legitimate warnings may be displayed too late and to the wrong people, but OTOH this is better than many false negatives and would increase precision.

src/libproc_macro/bridge/buffer.rs Outdated Show resolved Hide resolved
src/libproc_macro/bridge/closure.rs Outdated Show resolved Hide resolved
src/librustc_codegen_llvm/back/write.rs Outdated Show resolved Hide resolved
src/libstd/thread/local.rs Outdated Show resolved Hide resolved
src/test/ui/generic/generic-no-mangle.rs Outdated Show resolved Hide resolved
src/libproc_macro/bridge/client.rs Outdated Show resolved Hide resolved
@cramertj
Copy link
Member

cramertj commented Oct 7, 2019

r? @varkor

@rust-highfive rust-highfive assigned varkor and unassigned cramertj Oct 7, 2019
@davidtwco
Copy link
Member Author

Thanks for your comments @rkruppe, I’ll have time to revisit this later in the week and address them then.

@JohnCSimon JohnCSimon 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 Oct 12, 2019
@JohnCSimon
Copy link
Member

Ping from triage
@davidtwco Is this still a draft PR? Can you please post your status?
Thank you.

@davidtwco
Copy link
Member Author

@JohnCSimon This is still a draft PR. I’ve not had time to address the comments yet.

@davidtwco davidtwco force-pushed the issue-19834-improper-ctypes-in-extern-C-fn branch from c5f18a7 to 4788b84 Compare October 26, 2019 18:00
@davidtwco
Copy link
Member Author

I've updated the PR so that the improper ctypes lint does not trigger for generic extern "C" fns. This should cut down on the false positives (it can always be done as a follow-up) and let us proceed with this PR and lint on the majority of cases.

@davidtwco davidtwco marked this pull request as ready for review October 26, 2019 18:05
@eddyb
Copy link
Member

eddyb commented Oct 26, 2019

Are generic functions ignored, or just their type parameters considered FFI-safe for the purposes of the analysis?

@davidtwco
Copy link
Member Author

davidtwco commented Oct 26, 2019

Are generic functions ignored, or just their type parameters considered FFI-safe for the purposes of the analysis?

I skip functions with generics entirely. I suppose I could just ignore the generics and still lint on everything else. I don't think I can just consider the type parameters FFI-safe because the normalize_erasing_regions call in check_type_for_ffi_and_report_errors will panic because of them, before the call to check_type_for_ffi where I'd say they are FFI-safe.

// it is only OK to use this function because extern fns cannot have
// any generic types right now:
let ty = self.cx.tcx.normalize_erasing_regions(ParamEnv::reveal_all(), ty);

I suppose I could bail out early for type parameters before that call though..

@eddyb
Copy link
Member

eddyb commented Oct 26, 2019

That call shouldn't ICE if you use a valid ParamEnv (i.e. tcx.param_env(def_id)).

@davidtwco davidtwco force-pushed the issue-19834-improper-ctypes-in-extern-C-fn branch 2 times, most recently from 3558367 to 8b2d8e1 Compare October 26, 2019 19:37
@davidtwco
Copy link
Member Author

Thanks, @eddyb. I've updated the PR so that type parameters are considered FFI-safe and the rest of the function will continue to be linted. See the test snippet shown below:

https://github.com/rust-lang/rust/blob/8b2d8e19ec74f55af2cfa31bc642c5927c43fdeb/src/test/ui/lint/lint-ctypes-fn.rs#L164-L179

src/librustc_lint/types.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
@davidtwco davidtwco force-pushed the issue-19834-improper-ctypes-in-extern-C-fn branch from 8b2d8e1 to d0e440d Compare October 27, 2019 09:54
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 6, 2019
@bors
Copy link
Contributor

bors commented Nov 6, 2019

⌛ Testing commit 49e2403 with merge 3f0e164...

bors added a commit that referenced this pull request Nov 6, 2019
…n-C-fn, r=rkruppe

improper_ctypes: `extern "C"` fns

cc #19834. Fixes #65867.

This pull request implements the change [described in this comment](#19834 (comment)).

cc @rkruppe @varkor @shepmaster
@bors bors mentioned this pull request Nov 6, 2019
@bors
Copy link
Contributor

bors commented Nov 6, 2019

☀️ Test successful - checks-azure
Approved by: rkruppe
Pushing 3f0e164 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2019
@bors bors merged commit 49e2403 into rust-lang:master Nov 6, 2019
@davidtwco davidtwco deleted the issue-19834-improper-ctypes-in-extern-C-fn branch November 6, 2019 17:01
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 6, 2019
rustup improper_ctypes: `extern "C"` fns

cc rust-lang/rust#65134
changelog: none
ghedo added a commit to cloudflare/quiche that referenced this pull request Nov 7, 2019
This is actually a false positive, as the structs are only exposed to
FFI as pointers, so they are effectively opaque from the non-Rust
perspective

The warning also showed up in nightly after the following PR was merged:
rust-lang/rust#65134
ghedo added a commit to cloudflare/quiche that referenced this pull request Nov 7, 2019
This is actually a false positive, as the structs are only exposed to
FFI as pointers, so they are effectively opaque from the non-Rust
perspective

The warning showed up in nightly after the following PR was merged:
rust-lang/rust#65134
ghedo added a commit to cloudflare/quiche that referenced this pull request Nov 7, 2019
This is actually a false positive, as the structs are only exposed to
FFI as pointers, so they are effectively opaque from the non-Rust
perspective

The warning showed up in nightly after the following PR was merged:
rust-lang/rust#65134
@jethrogb
Copy link
Contributor

jethrogb commented Nov 13, 2019

I'm now getting a warning on

extern "C" fn _f(_a: *mut *mut std::sync::Mutex<()>) {}

Since the argument is just a thin raw pointer, I don't think the compiler should generate a warning here.

hanna-kruppe pushed a commit that referenced this pull request Nov 13, 2019
…in-extern-C-fn, r=rkruppe"

This reverts commit 3f0e164, reversing
changes made to 61a551b.
bors added a commit that referenced this pull request Nov 14, 2019
Revert #65134

To stop giving people on nightly reasons to `allow(improper_ctypes)` while tweaks to the lint are being prepared.

cc #66220
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 23, 2020
…es-declarations, r=lcnr,varkor

`improper_ctypes_definitions` lint

Addresses rust-lang#19834, rust-lang#66220, and rust-lang#66373.

This PR takes another attempt at rust-lang#65134 (reverted in rust-lang#66378). Instead of modifying the existing `improper_ctypes` lint to consider `extern "C" fn` definitions in addition to `extern "C" {}` declarations, this PR adds a new lint - `improper_ctypes_definitions` - which only applies to `extern "C" fn` definitions.

In addition, the `improper_ctype_definitions` lint differs from `improper_ctypes` by considering `*T` and `&T` (where `T: Sized`) FFI-safe (addressing rust-lang#66220).

There wasn't a clear consensus in rust-lang#66220 (where the issues with rust-lang#65134 were primarily discussed) on the approach to take, but there has [been some discussion in Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.2366220.20improper_ctypes.20definitions.20vs.20declarations/near/198903086). I fully expect that we'll want to iterate on this before landing.

cc @varkor + @shepmaster (from rust-lang#19834) @hanna-kruppe (active in discussing rust-lang#66220), @SimonSapin (rust-lang#65134 caused problems for Servo, want to make sure that this PR doesn't)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 25, 2020
…es-declarations, r=lcnr,varkor

`improper_ctypes_definitions` lint

Addresses rust-lang#19834, rust-lang#66220, and rust-lang#66373.

This PR takes another attempt at rust-lang#65134 (reverted in rust-lang#66378). Instead of modifying the existing `improper_ctypes` lint to consider `extern "C" fn` definitions in addition to `extern "C" {}` declarations, this PR adds a new lint - `improper_ctypes_definitions` - which only applies to `extern "C" fn` definitions.

In addition, the `improper_ctype_definitions` lint differs from `improper_ctypes` by considering `*T` and `&T` (where `T: Sized`) FFI-safe (addressing rust-lang#66220).

There wasn't a clear consensus in rust-lang#66220 (where the issues with rust-lang#65134 were primarily discussed) on the approach to take, but there has [been some discussion in Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.2366220.20improper_ctypes.20definitions.20vs.20declarations/near/198903086). I fully expect that we'll want to iterate on this before landing.

cc @varkor + @shepmaster (from rust-lang#19834) @hanna-kruppe (active in discussing rust-lang#66220), @SimonSapin (rust-lang#65134 caused problems for Servo, want to make sure that this PR doesn't)
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 25, 2020
…es-declarations, r=lcnr,varkor

`improper_ctypes_definitions` lint

Addresses rust-lang#19834, rust-lang#66220, and rust-lang#66373.

This PR takes another attempt at rust-lang#65134 (reverted in rust-lang#66378). Instead of modifying the existing `improper_ctypes` lint to consider `extern "C" fn` definitions in addition to `extern "C" {}` declarations, this PR adds a new lint - `improper_ctypes_definitions` - which only applies to `extern "C" fn` definitions.

In addition, the `improper_ctype_definitions` lint differs from `improper_ctypes` by considering `*T` and `&T` (where `T: Sized`) FFI-safe (addressing rust-lang#66220).

There wasn't a clear consensus in rust-lang#66220 (where the issues with rust-lang#65134 were primarily discussed) on the approach to take, but there has [been some discussion in Zulip](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.2366220.20improper_ctypes.20definitions.20vs.20declarations/near/198903086). I fully expect that we'll want to iterate on this before landing.

cc @varkor + @shepmaster (from rust-lang#19834) @hanna-kruppe (active in discussing rust-lang#66220), @SimonSapin (rust-lang#65134 caused problems for Servo, want to make sure that this PR doesn't)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for improper ctypes in all "stable ABIs"
10 participants