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 feature gate for raw_dylib. #63948

Open
wants to merge 2 commits into
base: master
from

Conversation

@crlf0710
Copy link
Contributor

commented Aug 27, 2019

This PR adds the feature gate for RFC 2627 (#58713). It doesn't contain the actual functionality.
Add I'm not sure whether i did it correctly, since this is the first time i did this.

r? @Centril

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

The job mingw-check of your PR failed (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.
2019-08-27T14:56:23.2462098Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-27T14:56:23.2671668Z ##[command]git config gc.auto 0
2019-08-27T14:56:23.2748195Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-27T14:56:23.2801636Z ##[command]git config --get-all http.proxy
2019-08-27T14:56:23.2935277Z ##[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/63948/merge:refs/remotes/pull/63948/merge
---
2019-08-27T14:56:58.7152759Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-27T14:56:58.7152901Z 
2019-08-27T14:56:58.7154230Z   git checkout -b <new-branch-name>
2019-08-27T14:56:58.7154449Z 
2019-08-27T14:56:58.7154641Z HEAD is now at 18555b180 Merge c7559554453b2e0de85c8fc710392c7e634fabd7 into 7e0afdad28c4d1154176df6d35a14e738ec311af
2019-08-27T14:56:58.7314462Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-27T14:56:58.7317192Z ==============================================================================
2019-08-27T14:56:58.7317274Z Task         : Bash
2019-08-27T14:56:58.7317311Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-27T15:04:13.1501019Z    Compiling rustc_macros v0.1.0 (/checkout/src/librustc_macros)
2019-08-27T15:04:21.8883220Z     Checking syntax_pos v0.0.0 (/checkout/src/libsyntax_pos)
2019-08-27T15:04:23.3693979Z     Checking rustc_errors v0.0.0 (/checkout/src/librustc_errors)
2019-08-27T15:04:24.5870884Z     Checking fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
2019-08-27T15:04:28.2870136Z error[E0609]: no field `link_ordinal` on type `&feature_gate::active::Features`
2019-08-27T15:04:28.2871214Z    --> src/libsyntax/feature_gate/builtin_attrs.rs:278:12
2019-08-27T15:04:28.2872065Z     |
2019-08-27T15:04:28.2873508Z 278 |     gated!(link_ordinal, Whitelisted, template!(Word), experimental!(link_ordinal)),
2019-08-27T15:04:28.2874186Z 
2019-08-27T15:04:32.3064606Z error: aborting due to previous error
2019-08-27T15:04:32.3065387Z 
2019-08-27T15:04:32.3066143Z For more information about this error, try `rustc --explain E0609`.
---
2019-08-27T15:04:32.3685604Z == clock drift check ==
2019-08-27T15:04:32.3710208Z   local time: Tue Aug 27 15:04:32 UTC 2019
2019-08-27T15:04:32.4639155Z   network time: Tue, 27 Aug 2019 15:04:32 GMT
2019-08-27T15:04:32.4639548Z == end clock drift check ==
2019-08-27T15:04:33.7973286Z ##[error]Bash exited with code '1'.
2019-08-27T15:04:33.8013816Z ##[section]Starting: Checkout
2019-08-27T15:04:33.8016278Z ==============================================================================
2019-08-27T15:04:33.8016663Z Task         : Get sources
2019-08-27T15:04:33.8016874Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@crlf0710 crlf0710 force-pushed the crlf0710:path_to_raw_dylib branch from c755955 to d783ca3 Aug 27, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

The job mingw-check of your PR failed (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.
2019-08-27T15:54:41.1679959Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-27T15:54:41.1966025Z ##[command]git config gc.auto 0
2019-08-27T15:54:41.2041510Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-27T15:54:41.2084112Z ##[command]git config --get-all http.proxy
2019-08-27T15:54:41.2215033Z ##[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/63948/merge:refs/remotes/pull/63948/merge
---
2019-08-27T15:55:17.7731580Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-27T15:55:17.7731614Z 
2019-08-27T15:55:17.7731802Z   git checkout -b <new-branch-name>
2019-08-27T15:55:17.7731827Z 
2019-08-27T15:55:17.7731885Z HEAD is now at d487c7846 Merge d783ca3f5ff4f84fdf45f21357b7892704ab1704 into 7e0afdad28c4d1154176df6d35a14e738ec311af
2019-08-27T15:55:17.7883547Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-27T15:55:17.7886356Z ==============================================================================
2019-08-27T15:55:17.7886416Z Task         : Bash
2019-08-27T15:55:17.7886464Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-27T16:03:46.7658164Z     Checking rustc_plugin_impl v0.0.0 (/checkout/src/librustc_plugin)
2019-08-27T16:03:47.0377925Z     Checking rustc_resolve v0.0.0 (/checkout/src/librustc_resolve)
2019-08-27T16:03:49.5401825Z     Checking rustc_privacy v0.0.0 (/checkout/src/librustc_privacy)
2019-08-27T16:03:50.1115803Z     Checking rustc_codegen_ssa v0.0.0 (/checkout/src/librustc_codegen_ssa)
2019-08-27T16:03:52.7906052Z error[E0004]: non-exhaustive patterns: `NativeRawDylib` not covered
2019-08-27T16:03:52.7906438Z    --> src/librustc_codegen_ssa/back/link.rs:312:15
2019-08-27T16:03:52.7916232Z 312 |         match lib.kind {
2019-08-27T16:03:52.7916232Z 312 |         match lib.kind {
2019-08-27T16:03:52.7920602Z     |               ^^^^^^^^ pattern `NativeRawDylib` not covered
2019-08-27T16:03:52.7929838Z     |
2019-08-27T16:03:52.7944321Z     = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
2019-08-27T16:03:52.7944444Z 
2019-08-27T16:03:52.8003467Z error[E0004]: non-exhaustive patterns: `NativeRawDylib` not covered
2019-08-27T16:03:52.8003801Z    --> src/librustc_codegen_ssa/back/link.rs:859:19
2019-08-27T16:03:52.8004310Z 859 |             match lib.kind {
2019-08-27T16:03:52.8004310Z 859 |             match lib.kind {
2019-08-27T16:03:52.8004610Z     |                   ^^^^^^^^ pattern `NativeRawDylib` not covered
2019-08-27T16:03:52.8004839Z     |
2019-08-27T16:03:52.8005134Z     = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
2019-08-27T16:03:52.8005174Z 
2019-08-27T16:03:52.8074648Z error[E0004]: non-exhaustive patterns: `NativeRawDylib` not covered
2019-08-27T16:03:52.8074993Z     --> src/librustc_codegen_ssa/back/link.rs:1279:15
2019-08-27T16:03:52.8075459Z 1279 |         match lib.kind {
2019-08-27T16:03:52.8075459Z 1279 |         match lib.kind {
2019-08-27T16:03:52.8075759Z      |               ^^^^^^^^ pattern `NativeRawDylib` not covered
2019-08-27T16:03:52.8075955Z      |
2019-08-27T16:03:52.8076262Z      = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
2019-08-27T16:03:52.8076304Z 
2019-08-27T16:03:52.8135466Z error[E0004]: non-exhaustive patterns: `NativeRawDylib` not covered
2019-08-27T16:03:52.8135853Z     --> src/librustc_codegen_ssa/back/link.rs:1646:19
2019-08-27T16:03:52.8136348Z 1646 |             match lib.kind {
2019-08-27T16:03:52.8136348Z 1646 |             match lib.kind {
2019-08-27T16:03:52.8136652Z      |                   ^^^^^^^^ pattern `NativeRawDylib` not covered
2019-08-27T16:03:52.8136867Z      |
2019-08-27T16:03:52.8137190Z      = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
2019-08-27T16:03:54.2242656Z error: aborting due to 4 previous errors
2019-08-27T16:03:54.2242755Z 
2019-08-27T16:03:54.2243214Z For more information about this error, try `rustc --explain E0004`.
2019-08-27T16:03:54.2528082Z error: Could not compile `rustc_codegen_ssa`.
---
2019-08-27T16:04:01.5740980Z == clock drift check ==
2019-08-27T16:04:01.5754927Z   local time: Tue Aug 27 16:04:01 UTC 2019
2019-08-27T16:04:01.8626186Z   network time: Tue, 27 Aug 2019 16:04:01 GMT
2019-08-27T16:04:01.8628845Z == end clock drift check ==
2019-08-27T16:04:03.4180571Z ##[error]Bash exited with code '1'.
2019-08-27T16:04:03.4215255Z ##[section]Starting: Checkout
2019-08-27T16:04:03.4216656Z ==============================================================================
2019-08-27T16:04:03.4216700Z Task         : Get sources
2019-08-27T16:04:03.4216760Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@Centril Centril added the F-raw_dylib label Aug 27, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

@rust-highfive rust-highfive assigned nagisa and unassigned Centril Aug 27, 2019

@crlf0710 crlf0710 force-pushed the crlf0710:path_to_raw_dylib branch 2 times, most recently from c4abdb8 to 9fdbcb3 Aug 27, 2019

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

LGTM

@retep998

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Well, at least it is some sort of progress. Nothing here that I can test or review yet.

@nagisa
Copy link
Contributor

left a comment

The gate implementation looks right, however to the best of my knowledge we avoid merging PRs that are not self-contained (as would be the case with this PR) as that may lead to dead code if the remaining changes are never made.

@crlf0710

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

I'm happy to implement more or even all of the RFC if i can. But i guess i need some mentoring, especially on the codegen part.
Welcome to leave notes on https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Sketching.20out.20a.20plan.20for.20implementing.20RFC.202627 . I appreciate your help :)

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

The gate implementation looks right, however to the best of my knowledge we avoid merging PRs that are not self-contained (as would be the case with this PR) as that may lead to dead code if the remaining changes are never made.

@nagisa Shouldn't be too hard to remove things if the other changes are never made in this case. We have an INCOMPLETE_FEATURES list in active.rs for features in a similar situation.

@JohnCSimon

This comment has been minimized.

Copy link

commented Sep 7, 2019

Ping from triage
@crlf0710 @nagisa @Centril
Can you please address the review comments?
Thank you!

@crlf0710

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

Opened #64263. Will rebase after that is merged.

@crlf0710 crlf0710 force-pushed the crlf0710:path_to_raw_dylib branch from 9fdbcb3 to abf6cc7 Sep 9, 2019

@crlf0710

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Rebased, addressed the review comments, and added link_ordinal attribute parsing.

@crlf0710 crlf0710 force-pushed the crlf0710:path_to_raw_dylib branch from 7ae2daa to 81e6f7b Sep 9, 2019

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

This looks entirely reasonable to me, and seems like a good start for further implementation.

I think it's up to someone other than me whether we want to merge a commit that provides the skeleton of support like this, but personally I think it's a good idea.

template!(List: "ordinal"),
raw_dylib,
experimental!(link_ordinal)
),

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member
Suggested change
),
),
@@ -275,7 +275,13 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
"the `link_args` attribute is experimental and not portable across platforms, \
it is recommended to use `#[link(name = \"foo\")] instead",
),

gated!(

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member

Please use the formatting as in the other invocations of gated!(...) i.e.:

Suggested change
gated!(
gated!(
link_ordinal, Whitelisted, template!(List: "ordinal"), raw_dylib,
experimental!(link_ordinal)
),
@@ -0,0 +1,8 @@
#[link(name="foo")]
extern {
#[link_ordinal(42)]

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member

Please adjust the indentation in this file.

@@ -0,0 +1,8 @@
#[link(name="foo")]

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member

Please move this and the other feature-gate file to src/test/ui/rfc-2627-raw-dylib/. More test files should be added there over time.

@@ -528,6 +528,9 @@ declare_features! (
/// Allows the use of or-patterns (e.g., `0 | 1`).
(active, or_patterns, "1.38.0", Some(54883), None),

// Allows the use of raw-dylibs (RFC 2627).
(active, raw_dylib, "1.39.0", Some(58713), None),

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member

Please add sym::raw_dyblib to INCOMPLETE_FEATURES below.

@@ -2704,6 +2733,15 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
codegen_fn_attrs.export_name = Some(name);
codegen_fn_attrs.link_name = Some(name);
}
if codegen_fn_attrs.link_name.is_some() && codegen_fn_attrs.link_ordinal.is_some() {
if let Some(span) = inline_span {

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member

An error is not emitted in the case of None -- why that?

@@ -2603,6 +2603,35 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, id: DefId) -> CodegenFnAttrs {
}
} else if attr.check_name(sym::link_name) {
codegen_fn_attrs.link_name = attr.value_str();
} else if attr.check_name(sym::link_ordinal) {
use syntax::ast::{Lit, LitIntType, LitKind};

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member

This branch should be refactored into its own function/method fn check_link_ordinal(...).

} else if attr.check_name(sym::link_ordinal) {
use syntax::ast::{Lit, LitIntType, LitKind};
let meta_item_list = attr.meta_item_list();
let sole_meta_lit = if let Some(meta_item_list) = &meta_item_list {

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member

This bit can be rewritten as:

let sole_meta_list = match attr.meta_item_list() {
    Some([item]) => item.literal(),
    _ => None,
};
tcx.sess.span_err(attr.span, &msg);
}
} else {
let msg = "illegal ordinal format in link_ordinal";

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member

This can probably be inlined into the method call when introducing fn check_link_ordinal.

@@ -96,6 +96,8 @@ pub enum NativeLibraryKind {
NativeStaticNobundle,
/// macOS-specific
NativeFramework,
/// windows dynamic library without import library

This comment has been minimized.

Copy link
@Centril

Centril Sep 9, 2019

Member
Suggested change
/// windows dynamic library without import library
/// Windows dynamic library without import library.
@Centril

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

☔️ The latest upstream changes (presumably #64303) 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
Projects
None yet
8 participants
You can’t perform that action at this time.