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.

Sign up for GitHub

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

Jump to bottom

Add c_enum_min_bits target spec field, use for arm-none and thumb-none targets #87922

Merged
merged 2 commits into from
Aug 13, 2021
Merged

Add c_enum_min_bits target spec field, use for arm-none and thumb-none targets #87922

merged 2 commits into from
Aug 13, 2021

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Aug 10, 2021 •
edited
Loading

Fixes #87917

Haven't tested this yet, still playing around.

This seems to fix the issue.

@Manishearth Manishearth requested a review from eddyb August 10, 2021 19:10
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Manishearth Manishearth force-pushed the c-enum-target-spec branch 2 times, most recently from af58c42 to d8fa2db Compare August 10, 2021 19:21
@Manishearth Manishearth requested a review from eddyb August 10, 2021 19:45
@Manishearth Manishearth assigned eddyb and unassigned jackh726 Aug 10, 2021
@Manishearth Manishearth marked this pull request as ready for review August 10, 2021 19:45
@Manishearth
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 10, 2021

⌛ Trying commit 0b973f7da664b296278bf401d198602a0f7cc45a with merge d6fefca960f43fece956faf72cb6b2ac248134a4...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Manishearth
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 10, 2021

⌛ Trying commit 6ab5094d93fb7fa48eda803f2fdf8aeb91c6435f with merge 42cd1bdeeddb458075fbc731d20575a4ceb6941a...

@Manishearth
Copy link
Member Author

@bors try

seems like it failed with "startup failure"

@bors
Copy link
Contributor

bors commented Aug 10, 2021

⌛ Trying commit 6ab5094d93fb7fa48eda803f2fdf8aeb91c6435f with merge 97161a833baea7332c5c23449259a8e2512afc53...

@Manishearth
Copy link
Member Author

@bors try

did a push, need a retrigger

@bors
Copy link
Contributor

bors commented Aug 10, 2021

⌛ Trying commit 3b7eda5aca1188f28da10e62c289bb596836c9fc with merge ed376b0496699bb1431b022ef1ea7edb8d45a013...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Manishearth
Copy link
Member Author

There's a try build at 97161a833baea7332c5c23449259a8e2512afc53 which can be used.

@Manishearth
Copy link
Member Author

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned eddyb Aug 11, 2021
@Amanieu Amanieu added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 11, 2021
@Amanieu
Copy link
Member

Amanieu commented Aug 11, 2021

Tagging for release notes since this is technically a breaking change.

eddyb
eddyb reviewed Aug 12, 2021
View reviewed changes
src/test/ui/layout/thumb-enum.rs
#[repr(C)]
enum P { Peach = 0x1000_0000isize, } //~ ERROR: layout_of

const TANGERINE: usize = 0x8100_0000; // hack to get negative numbers without negation operator!
Copy link
Member

Choose a reason for hiding this comment

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

Huh, why is this value here like this, you can just write -....

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, #![no_core] - the "without negation operator" part should probably mention #![no_core] and #[lang = "neg"], it's really confusing on its own.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied the other test

eddyb
eddyb reviewed Aug 12, 2021
View reviewed changes
src/test/ui/layout/thumb-enum.rs
Comment on lines +11 to +12
#[lang="sized"]
trait Sized {}
Copy link
Member

Choose a reason for hiding this comment

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

Huh, cool, I guess there is no "code", making this really easy.

eddyb
eddyb reviewed Aug 12, 2021
View reviewed changes
compiler/rustc_target/src/spec/armebv7r_none_eabi.rs
Comment on lines +23 to +24
// GCC and Clang default to 8 for arm-none here
c_enum_min_bits: 8,
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to have some deduplication between all the arm*-none-eabi(hf) target specs, but it doesn't have to happen in this PR.

eddyb
eddyb approved these changes Aug 12, 2021
View reviewed changes
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Implementation LGTM, but I don't know what our policy on adding a new target spec field is.

@nagisa
Copy link
Member

nagisa commented Aug 12, 2021

Targets and interfaces to targets (custom target schemas) aren't considered stable, so from the target changes' perspective this seems good.

We're also making Rust more compatible with other targets than less-so. The only use-case I can see breaking here is if Rust code built with different rustc versions is mixed. But it does fix interop with other native toolchains so that seems like an overall win.

@bors r=nagisa,eddyb

@bors
Copy link
Contributor

bors commented Aug 12, 2021

📌 Commit 5fbb5c93bd38562f4b86327573d5850bb1a3d7f2 has been approved by nagisa,eddyb

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2021
@nagisa
Copy link
Member

nagisa commented Aug 12, 2021

Ah, please squash the commits together. r=nagisa,eddyb after

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 12, 2021
@Manishearth
Copy link
Member Author

@bors r=nagisa,eddyb

@bors
Copy link
Contributor

bors commented Aug 12, 2021

📌 Commit 4c0e424 has been approved by nagisa,eddyb

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2021
@Manishearth Manishearth mentioned this pull request Aug 12, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2021
@bors
Auto merge of rust-lang#87980 - Manishearth:rollup-vkuix3y, r=Manishe…
61885df 
…arth

Rollup of 4 pull requests

Successful merges:

 - rust-lang#87916 (Implement `black_box` using intrinsic)
 - rust-lang#87922 (Add c_enum_min_bits target spec field, use for arm-none and thumb-none targets)
 - rust-lang#87953 (Improve formatting of closure capture migration suggestion for multi-line closures.)
 - rust-lang#87965 (Silence non_fmt_panic from external macros.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 692833a into rust-lang:master Aug 13, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 13, 2021
@Manishearth Manishearth deleted the c-enum-target-spec branch August 13, 2021 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@nagisa nagisa

Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.

arm-none-eabi (etc) have incorrect repr(C) enum layout (AAPCS vs AAPCS-linux)
9 participants
@Manishearth @rust-highfive @rust-log-analyzer @bors @Amanieu @nagisa @eddyb @jackh726 @rustbot