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

Add a lint to catch clashing extern fn declarations. #70946

Merged
merged 4 commits into from
Jun 21, 2020

Conversation

jumbatm
Copy link
Contributor

@jumbatm jumbatm commented Apr 9, 2020

Closes #69390.

Adds lint clashing_extern_decl to detect when, within a single crate, an extern function of the same name is declared with different types. Because two symbols of the same name cannot be resolved to two different functions at link time, and one function cannot possibly have two types, a clashing extern declaration is almost certainly a mistake.

This lint does not run between crates because a project may have dependencies which both rely on the same extern function, but declare it in a different (but valid) way. For example, they may both declare an opaque type for one or more of the arguments (which would end up distinct types), or use types that are valid conversions in the language the extern fn is defined in. In these cases, we can't say that the clashing declaration is incorrect.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2020
@jumbatm
Copy link
Contributor Author

jumbatm commented Apr 9, 2020

Just as a reminder, per #69390 (comment), this PR should go through a crater-check run before merging (which should also let us see if changing the lint to be Deny from the get-go would break many current crates).

@eddyb
Copy link
Member

eddyb commented Apr 9, 2020

r? @nagisa cc @hanna-kruppe (LGTM at a glance though)

@rust-highfive rust-highfive assigned nagisa and unassigned eddyb Apr 9, 2020
@nagisa
Copy link
Member

nagisa commented Apr 13, 2020

This is missing a test (and I think lint will fail to correctly handle) for two two not clashing declarations that use independently declared types. That is:

mod one {
    #[repr(C)] struct Banana { weight: u64 }
    extern "C" { fn weigh_banana(count: *const Banana) -> u64; }
}

mod two {
    #[repr(C)] struct Banana { weight: u64 } // note: distinct type
    // For a weirder corner case (may still be valid depending on how C code is written):
    // #[repr(C)] struct Banana { weight: u64, some_optional_field: u64 }
    extern "C" { fn weigh_banana(count: *const Banana) -> u64; }
}

@jumbatm jumbatm changed the title Add a lint to catch clashing extern declarations. Add a lint to catch clashing extern fn declarations. Apr 19, 2020
src/librustc_lint/builtin.rs Outdated Show resolved Hide resolved
@bjorn3
Copy link
Member

bjorn3 commented Apr 19, 2020

Should this also check across multiple crates?

@jumbatm
Copy link
Contributor Author

jumbatm commented Apr 19, 2020

Current thinking is no, because two crates may bind to the same extern function with signatures that are different, but compatible under that language or ABI.

@bors
Copy link
Contributor

bors commented Apr 26, 2020

☔ The latest upstream changes (presumably #71566) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented May 9, 2020

☔ The latest upstream changes (presumably #72036) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented May 16, 2020

☔ The latest upstream changes (presumably #72276) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
##[section]Starting: Linux mingw-check
##[section]Starting: Initialize job
Agent name: 'Azure Pipelines 2'
Agent machine name: 'fv-az578'
Current agent version: '2.168.2'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200430.2
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200430.2/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.2)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/f16730d5-d62e-4189-ad81-c967b265d4fd.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/70946/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/70946/merge:refs/remotes/pull/70946/merge
---
 ---> 3adb0605cc65
Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
 ---> Using cache
 ---> 28dbc326cb7f
Step 7/7 : ENV SCRIPT python3 ../x.py test src/tools/expand-yaml-anchors &&            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python3 ../x.py build --stage 0 src/tools/build-manifest &&            python3 ../x.py test --stage 0 src/tools/compiletest &&            python3 ../x.py test src/tools/tidy &&            python3 ../x.py doc --stage 0 src/libstd &&            /scripts/validate-toolstate.sh
 ---> 537a01811900
Successfully built 537a01811900
Successfully tagged rust-ci:latest
Built container sha256:537a018119009dc218456238dec90b5530050db1e2a1e166550c218003f6159d
---
    Checking rustc_feature v0.0.0 (/checkout/src/librustc_feature)
    Checking fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
    Checking rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
    Checking rustc_hir v0.0.0 (/checkout/src/librustc_hir)
    Checking rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
    Checking chalk-rust-ir v0.10.0
    Checking rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
    Checking rustc_parse v0.0.0 (/checkout/src/librustc_parse)
    Checking rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
    Checking rustc_lint v0.0.0 (/checkout/src/librustc_lint)
error[E0531]: cannot find tuple struct or tuple variant `UnnormalizedProjection` in this scope
    --> src/librustc_lint/builtin.rs:2195:20
     |
2195 |                 | (UnnormalizedProjection(..), UnnormalizedProjection(..))

error[E0531]: cannot find tuple struct or tuple variant `UnnormalizedProjection` in this scope
    --> src/librustc_lint/builtin.rs:2195:48
     |
     |
2195 |                 | (UnnormalizedProjection(..), UnnormalizedProjection(..))

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0531`.
For more information about this error, try `rustc --explain E0531`.
error: could not compile `rustc_lint`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--color" "always" "--features" " llvm" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:04:32
== clock drift check ==
  local time: Sun May 17 14:36:26 UTC 2020
  local time: Sun May 17 14:36:26 UTC 2020
  network time: Sun, 17 May 2020 14:36:26 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/70946/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/70946/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3818) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@jumbatm
Copy link
Contributor Author

jumbatm commented May 18, 2020

Hey @nagisa, this is ready for re-review.

@nagisa
Copy link
Member

nagisa commented May 19, 2020

This overall LGTM now. I think it does get the balance right.

Can you please clean up the commits/history a little? Squashing some together, especially those that refer to rustfmt, etc would be ideal.

r=me once that's done.

@nagisa
Copy link
Member

nagisa commented May 20, 2020

@bors r+

@bors
Copy link
Contributor

bors commented May 20, 2020

📌 Commit 337abf1 has been approved by nagisa

@bors
Copy link
Contributor

bors commented May 20, 2020

🌲 The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened

@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 May 20, 2020
@jumbatm
Copy link
Contributor Author

jumbatm commented May 20, 2020

Should we do a crater-check run first? Given this fixes a soundness issue, I'd like to make this lint Deny by default, but want to see if it'd break any existing crates.

RalfJung added a commit to RalfJung/rust that referenced this pull request May 21, 2020
Add a lint to catch clashing `extern` fn declarations.

Closes rust-lang#69390.

Adds lint `clashing_extern_decl` to detect when, within a single crate, an extern function of the same name is declared with different types. Because two symbols of the same name cannot be resolved to two different functions at link time, and one function cannot possibly have two types, a clashing extern declaration is almost certainly a mistake.

This lint does not run between crates because a project may have dependencies which both rely on the same extern function, but declare it in a different (but valid) way. For example, they may both declare an opaque type for one or more of the arguments (which would end up distinct types), or use types that are valid conversions in the language the extern fn is defined in. In these cases, we can't say that the clashing declaration is incorrect.

r? @eddyb
This lint checks that all declarations for extern fns of the same name
are declared with the same types.
- Allow ClashingExternDecl for lint-dead-code-3
- Update test case for rust-lang#5791
- Update test case for rust-lang#1866
- Update extern-abi-from-macro test case
@jumbatm
Copy link
Contributor Author

jumbatm commented Jun 20, 2020

Hey @nagisa, can I get another review? Fingers crossed this can get merged soon.

@nagisa
Copy link
Member

nagisa commented Jun 20, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jun 20, 2020

📌 Commit 556b7ba has been approved by nagisa

@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 20, 2020
@bors
Copy link
Contributor

bors commented Jun 20, 2020

⌛ Testing commit 556b7ba with merge 4399d4f09c61ea4b3a9070ddc591ab4d72a563d9...

@bors
Copy link
Contributor

bors commented Jun 20, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 20, 2020
@Dylan-DPC-zz
Copy link

going to retry again

@bors retry

@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 20, 2020
@bors
Copy link
Contributor

bors commented Jun 21, 2020

⌛ Testing commit 556b7ba with merge 228a0ed...

@bors
Copy link
Contributor

bors commented Jun 21, 2020

☀️ Test successful - checks-azure
Approved by: nagisa
Pushing 228a0ed to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2020
@bors bors merged commit 228a0ed into rust-lang:master Jun 21, 2020
@jumbatm jumbatm deleted the clashing-extern-decl branch June 26, 2020 06:53
@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 27, 2020

The lint doesn't follow the lint naming conventions - https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#lints.

It should preferably be renamed clashing_extern_decl -> clashing_extern_declarations until it's on stable.

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.

Miscompilation with clashing symbol fn names of different types