Implementation of RFC2867 #76260

xd009642 · 2020-09-02T21:33:30Z

So I've started work on this, I think my next steps are to make use of the instruction_set value in the llvm codegen but this is the point where I begin to get a bit lost. I'm looking at the code but it would be nice to have some guidance on what I've currently done and what I'm doing next 😄

rust-highfive · 2020-09-02T21:33:34Z

r? @nikomatsakis

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

ketsuban · 2020-09-06T22:47:45Z

I think at minimum the following targets will also need has_thumb_interworking set to true.

armv5te-unknown-linux-gnueabi
armv5te-unknown-linux-musleabi
thumbv4t-none-eabi

Someone who knows about platforms supporting ARMv6 and ARMv7 will have to sort through the rest. (@jonas-schievink?)

src/librustc_codegen_llvm/attributes.rs

src/librustc_typeck/collect.rs

src/librustc_codegen_llvm/attributes.rs

xd009642 · 2020-09-07T22:08:00Z

Responded to feedback, also aimed to improve some of the error messages in collect.rs

jonas-schievink

Looks pretty good now! Some tests should be added though (and some manual testing is probably useful too)

src/librustc_typeck/collect.rs

xd009642 · 2020-09-07T23:13:41Z

@jonas-schievink where should I look to for examples of tests in the code base covering similar areas?

jonas-schievink · 2020-09-08T09:42:33Z

I think for the initial version of this, it's enough to check that the attribute is accepted and rejected correctly. For that, you can write UI tests in src/test/ui/instruction-set. You can check https://rustc-dev-guide.rust-lang.org/tests/adding.html for details.

jonas-schievink · 2020-09-08T09:43:13Z

Oh, I also suggest rebasing this PR to fix the merge conflicts, then CI can run tests for you.

xd009642 · 2020-09-09T19:54:02Z

@jonas-schievink So I started working on the tests and after seeing CI fail started running the tests locally. I'm just wondering if there's a way to run them for a different architecture? Otherwise I'll just get the architecture doesn't support instruction_set errors. I see there's also some ui tests with ignore-arm etc at the top of the test file - is there any documentation on what comments are available to change when a test is ran or not?

xd009642 · 2020-09-14T20:54:37Z

@jonas-schievink I'd like to try and get this sorted by this week if possible but I might need some help with the UI tests to figure out how to get them to just run with the intended architectures.. Also should I add a UI test to just run with intel so I can get the error message for arch doesn't support instruction_set 🤔 (I think yes so I'll probably just start that soon-ish)

mati865 · 2020-09-15T07:04:51Z

src/test/ui/error-codes/E0775.rs

@@ -0,0 +1,11 @@
+// only-armv4t-unknown-linux-gnueabi
+// only-armv5te-unknown-linux-gnueabi
+// only-armv5te-unknown-linux-musleabi


Are you sure you can stack only like that?
I recall it did not work and tests like that have been changed to use different conditions.

I have no idea, I couldn't really find any documentation on how the comments worked and figured it was worth trying before writing the mass of ignore-* I'd need to do to run using just those 3 architectures

Judging by the comment it won't run this test on any target:

rust/src/tools/compiletest/src/header.rs

Line 53 in 6cae281

// we should check if any only-<platform> exists and if it exists

xd009642 · 2020-09-15T07:16:54Z

So I guess I need to add ignore- for everything not armv4 or armv5 then, unless there's an easier way?

mati865 · 2020-09-15T07:26:29Z

I don't know how to solve it and have very limited search ability on the mobile.

xd009642 · 2020-09-15T07:53:01Z

@mati865 okay thanks for the pointer though, I'll try the ignores at some point later, just trying to get the CI to the point of running those tests first 😬

xd009642 · 2020-09-15T21:08:07Z

src/test/ui/feature-gate-isa_attribute.rs

@@ -0,0 +1,35 @@
+// ignore-aarch64


@jonas-schievink @mati865 I don't know if either of you have time to have a look at this. But I can't figure out all the ignores I need to stop this running on architectures other than the arm v4 and v5 targets

It should be fine to just run it on one of the Arm architectures

Oh that would make things easier, I was trying to test on all relevant arm targets

src/test/ui/feature-gate-isa_attribute.rs

xd009642 · 2020-09-17T21:10:00Z

compiler/rustc_feature/src/builtin_attrs.rs

@@ -331,6 +331,8 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
        optimize, AssumedUsed, template!(List: "size|speed"), optimize_attribute,
        experimental!(optimize),
    ),
+    // RFC 2867
+    gated!(instruction_set, AssumedUsed, template!(List: "arm::a32|arm::t32"), isa_attribute, experimental!(instruction_set)),


@jonas-schievink so I have a feeling this is causing one of my issues with the E0775 error code test?

error[E0658]: the `#[instruction_set]` attribute is an experimental feature --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:15478:1 | 2 | #[instruction_set()] // error: expected one argument | ^^^^^^^^^^^^^^^^^^^^ | = note: see issue #74727 <https://github.com/rust-lang/rust/issues/74727> for more information = help: add `#![feature(isa_attribute)]` to the crate attributes to enable error[E0776]: target does not support `#[instruction_set]` --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:15478:1 | 2 | #[instruction_set()] // error: expected one argument | ^^^^^^^^^^^^^^^^^^^^ error: aborting due to 2 previous errors

I tried to do it with Word as the element instead of List previously but it didn't seem to work, might have been multiple issues working together though 🤔

You should use #![feature(instruction_set)] in the test to get rid of the first error.

The second error just looks like architecture support is checked before attribute syntax, which matches what your code is doing. I'm not sure how much validation is actually derived from the template! (the string is used in the error message, but might not actually be used to check attribute syntax).

Oh then I made a mistake (I guess). The RFC says the feature gate is isa_attribute and the attribute is instruction_set.

Ah, right, #![feature(isa_attribute)] should also fix this then. I think the RFC initially proposed #[isa] instead of #[instruction_set].

Change the check so it does it the other way around?

This codes outdated I changed it to gated!(isa_attribute, AssumedUsed, template!(Word), isa_attribute, experimental!(instruction_set)), but I still get the not supported on this architecture in the E0775 ui test

Your code in typeck first checks whether the architecture supports #[instruction_set], and only then check whether it is syntactically correct. There is no architecture check done in builtin_attrs.rs.

So with the E0775.rs ui test I have // only-armv4t-unknown-linux-gnueabi at the top of the file. And I thought that meant I'd only get the compiler errors for that architecture. Because I wouldn't expect the error messages to be the same for an unsupported arch as a supported arch, and checking thumb_interworking first seems to be the quickest and most-general check to do first (arm v4/v5 isn't a lot of users). I could change it to do it the other way round and fix the errors that way if that's the better/easiest/preferred approach

I think that would make sense. I'd usually expect syntax to be validated first, and independently of the target, and then have semantic checks performed.

jyn514 · 2020-10-07T23:00:55Z

Ah oops I was looking at the test failure from yesterday. That should be fine then.

jyn514 · 2020-10-07T23:02:10Z

I've just been doing ./x.py test

You can make this take less time with x.py test src/tools/error_index_generator src/test/ui/error-codes. I don't recommend running all 10k tests every time.

xd009642 · 2020-10-07T23:17:48Z

yeah I tried doing filtering on them like with cargo test but that didn't seem to work. It wasn't so bad when it failed early on 😅.

So the current error_code doctest issue is to do with the architecture it's ran on not supporting arm:a32 or arm::t32. I'll add a cfg_attr to it so it works. But do you want it to encompass all the targets it works on, or just pick one of them? And also should I mention which targets do support it or just leave that off because it's discoverable elsewhere i.e. the tracking issue?

jyn514 · 2020-10-07T23:31:12Z

I'm not the person to ask, but #76260 (comment) sounds like only one target is fine.

should I mention which targets do support it

This seems like the list could grow a lot over time - maybe point to the tracking issue instead?

xd009642 · 2020-10-08T11:42:50Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

jyn514 · 2020-10-08T13:06:50Z

r? @jonas-schievink

compiler/rustc_middle/src/middle/codegen_fn_attrs.rs

compiler/rustc_typeck/src/collect.rs

xd009642 · 2020-10-08T20:59:08Z

@jonas-schievink all the feedback should be addressed now 👍

jonas-schievink · 2020-10-08T21:03:48Z

Awesome! Will approve once CI passes

jyn514 · 2020-10-08T21:07:45Z

@xd009642 do you mind squashing the commit history a little? 52 commits is a lot 😅

jyn514 · 2020-10-08T21:08:06Z

There are instructions at https://rustc-dev-guide.rust-lang.org/git.html#rebasing

xd009642 · 2020-10-08T21:14:28Z

Yeah I can do that tomorrow morning 👍

jyn514 · 2020-10-08T21:15:56Z

gazes longingly at rust-lang/triagebot#821

xd009642 · 2020-10-08T22:59:18Z

@jonas-schievink @jyn514 squashed and I fixed the mistake in the stderr post case fix. Just couldn't wait until tomorrow 😄

jyn514 · 2020-10-08T23:35:22Z

@bors r=jonas-schievink

bors · 2020-10-08T23:35:24Z

📌 Commit bdb3f77 has been approved by jonas-schievink

bors · 2020-10-09T00:29:52Z

⌛ Testing commit bdb3f77 with merge 03ef8a0...

bors · 2020-10-09T02:27:22Z

☀️ Test successful - checks-actions, checks-azure
Approved by: jonas-schievink
Pushing 03ef8a0 to master...

rust-highfive assigned nikomatsakis Sep 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2020

jonas-schievink added the O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Sep 2, 2020

xd009642 mentioned this pull request Sep 6, 2020

Tracking Issue for #[instruction_set] attribute (RFC 2867) #74727

Closed

3 tasks

jonas-schievink reviewed Sep 7, 2020

View reviewed changes

src/librustc_codegen_llvm/attributes.rs Outdated Show resolved Hide resolved

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved

jonas-schievink reviewed Sep 7, 2020

View reviewed changes

src/librustc_codegen_llvm/attributes.rs Outdated Show resolved Hide resolved

jonas-schievink reviewed Sep 7, 2020

View reviewed changes

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

xd009642 marked this pull request as ready for review September 14, 2020 21:33

mati865 reviewed Sep 15, 2020

View reviewed changes

xd009642 commented Sep 15, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

xd009642 commented Sep 15, 2020

View reviewed changes

src/test/ui/feature-gate-isa_attribute.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

xd009642 commented Sep 17, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

rust-highfive assigned jonas-schievink and unassigned nikomatsakis Oct 8, 2020

jonas-schievink reviewed Oct 8, 2020

View reviewed changes

xd009642 added 2 commits October 8, 2020 23:32

Implement the instruction_set attribute

a6e2b63

Fix typo in error code

bdb3f77

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 Oct 8, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 9, 2020

bors merged commit 03ef8a0 into rust-lang:master Oct 9, 2020

rustbot added this to the 1.49.0 milestone Oct 9, 2020

bors mentioned this pull request Oct 9, 2020

Implement const expressions and patterns (RFC 2920) #77124

Merged

niclashoyer mentioned this pull request Dec 10, 2020

Implementer Wanted for Instruction Set RFC rust-embedded/wg#496

Closed

akien-mga mentioned this pull request Mar 24, 2021

Doc tests fail on armv7 for error codes E0778 and E0779 (instruction_set attribute) #83453

Open

Implementation of RFC2867 #76260

Implementation of RFC2867 #76260

Conversation

xd009642 commented Sep 2, 2020

rust-highfive commented Sep 2, 2020

ketsuban commented Sep 6, 2020

xd009642 commented Sep 7, 2020

jonas-schievink left a comment

Choose a reason for hiding this comment

xd009642 commented Sep 7, 2020

jonas-schievink commented Sep 8, 2020

jonas-schievink commented Sep 8, 2020

This comment has been minimized.

xd009642 commented Sep 9, 2020

This comment has been minimized.

xd009642 commented Sep 14, 2020

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xd009642 commented Sep 15, 2020

mati865 commented Sep 15, 2020

xd009642 commented Sep 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

jyn514 commented Oct 7, 2020

jyn514 commented Oct 7, 2020 • edited Loading

xd009642 commented Oct 7, 2020

jyn514 commented Oct 7, 2020

xd009642 commented Oct 8, 2020

jyn514 commented Oct 8, 2020

xd009642 commented Oct 8, 2020

jonas-schievink commented Oct 8, 2020

jyn514 commented Oct 8, 2020

jyn514 commented Oct 8, 2020

xd009642 commented Oct 8, 2020

jyn514 commented Oct 8, 2020

xd009642 commented Oct 8, 2020

jyn514 commented Oct 8, 2020

bors commented Oct 8, 2020

bors commented Oct 9, 2020

bors commented Oct 9, 2020

jyn514 commented Oct 7, 2020 •

edited

Loading