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 future incompatibility lint for #[link] without arguments #57139

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@varkor
Copy link
Member

varkor commented Dec 26, 2018

  • Add a lint invalid_link_arguments for #[link = "foo"] (rather than #[link(name = "foo")]).
  • Add a lint for duplicate arguments.
  • Improve feature gate diagnostics for #[link].
    Fixes #53901.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 26, 2018

r? @davidtwco

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

@varkor varkor changed the title Handle #[link] without arguments Report error for #[link] without arguments Dec 26, 2018

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 27, 2018

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.
travis_time:end:08580ed0:start=1545913518919896791,finish=1545913574589105804,duration=55669209013
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0

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 @TimNN. (Feature Requests)

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 27, 2018

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.
travis_time:end:1c661540:start=1545920622517707585,finish=1545920686392833840,duration=63875126255
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0

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 @TimNN. (Feature Requests)

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Dec 28, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2018

📌 Commit 3a5a683 has been approved by davidtwco

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2018

⌛️ Testing commit 3a5a683 with merge 30b2b1a...

bors added a commit that referenced this pull request Dec 28, 2018

Auto merge of #57139 - varkor:link-bad-args, r=davidtwco
Report error for #[link] without arguments

- Report an error for `#[link = "foo"]` (rather than `#[link(name = "foo")]`).
- Report an error for duplicate arguments.
- Improve feature gate diagnostics for `#[link]`.
Fixes #53901.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 28, 2018

💔 Test failed - status-travis

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 28, 2018

Your PR failed on Travis (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.
Attempting to download s3://rust-lang-ci-sccache2/docker/3b048215294fe3bf906d8c3c53525b5cbe6afdf1348cc1673b84252ca556549092fcc34b07065ccc939014f25eb64389c0131c5dfb2bf78eb70856ef1c3a7413
[00:00:07] Attempting with retry: curl -f -L -C - -o /tmp/rustci_docker_cache https://s3-us-west-1.amazonaws.com/rust-lang-ci-sccache2/docker/3b048215294fe3bf906d8c3c53525b5cbe6afdf1348cc1673b84252ca556549092fcc34b07065ccc939014f25eb64389c0131c5dfb2bf78eb70856ef1c3a7413
[00:00:07]   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
[00:00:07]                                  Dload  Upload   Total   Spent    Left  Speed
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

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 @TimNN. (Feature Requests)

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Dec 28, 2018

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 29, 2018

⌛️ Testing commit 3a5a683 with merge c1cbc73...

bors added a commit that referenced this pull request Dec 29, 2018

Auto merge of #57139 - varkor:link-bad-args, r=davidtwco
Report error for #[link] without arguments

- Report an error for `#[link = "foo"]` (rather than `#[link(name = "foo")]`).
- Report an error for duplicate arguments.
- Improve feature gate diagnostics for `#[link]`.
Fixes #53901.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 29, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 29, 2018

The job x86_64-gnu-aux of your PR failed on Travis (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.
[01:47:59]    Compiling euclid v0.15.2
[01:48:00]    Compiling ron v0.1.3
[01:48:00]    Compiling serde_json v1.0.2
[01:48:01]    Compiling webrender_api v0.53.0 (/checkout/obj/build/ct/webrender/webrender_api)
[01:48:03] error[E0459]: #[link(...)] specified without arguments
[01:48:03]  --> /cargo/registry/src/github.com-1ecc6299db9ec823/servo-glutin-0.12.0/src/api/dlopen.rs:9:1
[01:48:03]   |
[01:48:03] 9 | #[link="dl"]
[01:48:03]   | ^^^^^^^^^^^^ help: specify a `name` argument instead: `#[link(name = "dl")]`
[01:48:03] error: aborting due to previous error
[01:48:03] 
[01:48:03] For more information about this error, try `rustc --explain E0459`.
[01:48:03] error: Could not compile `servo-glutin`.
---
[01:48:18] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/cargotest" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "/checkout/obj/build/ct"
[01:48:18] expected success, got: exit code: 101
[01:48:18] 
[01:48:18] 
[01:48:18] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/test/pretty src/test/run-pass/pretty src/test/run-fail/pretty src/test/run-pass-valgrind/pretty src/test/run-pass-fulldeps/pretty src/test/run-fail-fulldeps/pretty src/tools/cargo src/tools/cargotest
[01:48:18] Build completed unsuccessfully in 0:34:18
[01:48:18] Makefile:50: recipe for target 'check-aux' failed
[01:48:18] make: *** [check-aux] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0424c9b8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sat Dec 29 06:03:08 UTC 2018
---
travis_time:end:14404cba:start=1546063391794823456,finish=1546063391806248081,duration=11424625
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00567b1f
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:start:crashlog
obj/cores/core.3691.!checkout!obj!build!x86_64-unknown-linux-gnu!stage2!bin!rustc
[New LWP 3696]
[New LWP 3691]
warning: Could not load shared library symbols for 8 libraries, e.g. /lib/x86_64-linux-gnu/libc.so.6.
Use the "info sharedlibrary" command to see the complete listing.
Do you need "set solib-search-path" or "set sysroot"?
Core was generated by `rustc --crate-name foo src/lib.rs --color never --crate-type lib --emit=dep-inf'.
Program terminated with signal SIGABRT, Aborted.
#0  0x00007f79f8e92428 in ?? ()
#0  0x00007f79f8e92428 in ?? ()
#1  0x00007f79f8e9402a in ?? ()
#2  0x0000000000000020 in ?? ()
#3  0x0000000000000000 in ?? ()
travis_time:end:00567b1f:start=1546063391813905637,finish=1546063393412846429,duration=1598940792
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:05fad18b
travis_time:start:05fad18b
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:307ae551
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 29, 2018

Servo's broken.

varkor added a commit to varkor/glutin that referenced this pull request Dec 29, 2018

@varkor varkor referenced this pull request Dec 29, 2018

Closed

Fix broken `link` in dlopen #148

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 29, 2018

This PR breaks code that runs on the Stable channel. What makes this breaking change acceptable?

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 29, 2018

@SimonSapin: I was just going to make a comment noting this. I'm could change this to a lint instead, so it won't break dependencies. It does seem reasonable to make this deny-by-default, though?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 29, 2018

A lint that is affected by --cap-lints is less bad, but deny by default still breaks code on Stable that didn’t opt into #[deny(warnings)].

#53901 doesn’t say much, what’s wrong with #[link = "dl"]?

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 29, 2018

#53901 doesn’t say much, what’s wrong with #[link = "dl"]?

When #[link] appears without an argument list (which includes #[link = "anything"]), it's just completely ignored:

None => continue,

The presence of silently-failing #[link] attributes seem like quite a bad thing.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 29, 2018

I agree that silently doing nothing is not good, but ecosystem impact is also something to consider. I think this needs at least a crater run, and perhaps some team discussion. Or is there an established policy or a precedent on how to deal with code that compiles on Stable but is likely incorrect, without being unsound?

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 31, 2018

(I've nominated the issue for discussion.)

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Dec 31, 2018

I am strongly in favor of having this lint.

If I recall correctly, the correct procedure should be a warn by default lint first, later upgraded to a deny by default lint once the major offenders have been fixed, and finally converted into a hard error in the distant future once crater comes up clean (or at the very least the remaining breakages are considered acceptable).

@varkor varkor force-pushed the varkor:link-bad-args branch from 3a5a683 to 9917de1 Jan 3, 2019

@varkor varkor changed the title Report error for #[link] without arguments Add lint for #[link] without arguments Jan 3, 2019

@varkor varkor changed the title Add lint for #[link] without arguments Add future incompatibility lint for #[link] without arguments Jan 3, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 10, 2019

This is mostly subsumed by #57321.
I suggest to wait until that PR lands and then report remaining mistakes (like duplicate kinds) like errors, crater found no regressions for them.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 16, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment