-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Use informational target machine for metadata #58605
Conversation
This does not seem to fix #58323. Here's the backtrace I'm getting:
In particular it's this call that is causing the problem: rust/src/librustc_codegen_ssa/base.rs Line 919 in 1349c84
The query is invoked here: rust/src/librustc_codegen_ssa/back/write.rs Line 1025 in 1349c84
|
039c0a5
to
aae7dbd
Compare
@michaelwoerister can you check this again? |
Hm, still failing. It seems that this evaluates to |
aae7dbd
to
aaba4fb
Compare
This should now work. |
☔ The latest upstream changes (presumably #58728) made this pull request unmergeable. Please resolve the merge conflicts. |
It does work r=me after a rebase. |
aaba4fb
to
02c4c28
Compare
What the actual f happened here? |
Damn is GitHub UI confusing. |
config::OptLevel::No | ||
} else { | ||
tcx.backend_optimization_level(LOCAL_CRATE) | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're not putting this "don't optimize if no codegen" into the backend_optimization_level query itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally a fix would have happened all the way down inside collect_and_partition_mono_items
(which would take care to not ICE when no codegen is happening), however that change is more involved than this and, given that I haven’t setup myself for reproduction, I wasn’t in a place to make such a change.
@bors r=michaelwoerister |
📌 Commit d89c2f6 has been approved by |
…oerister Use informational target machine for metadata Since there is nothing to optimise there... Should fix rust-lang#58323 but haven’t tested locally. r? @michaelwoerister
…oerister Use informational target machine for metadata Since there is nothing to optimise there... Should fix rust-lang#58323 but haven’t tested locally. r? @michaelwoerister
…oerister Use informational target machine for metadata Since there is nothing to optimise there... Should fix rust-lang#58323 but haven’t tested locally. r? @michaelwoerister
…oerister Use informational target machine for metadata Since there is nothing to optimise there... Should fix rust-lang#58323 but haven’t tested locally. r? @michaelwoerister
@bors retry |
Use informational target machine for metadata Since there is nothing to optimise there... Should fix #58323 but haven’t tested locally. r? @michaelwoerister
☀️ Test successful - checks-travis, status-appveyor |
This wasn't backported to beta, so the regression made its way to stable. @rust-lang/compiler nominating for stable backport. @rustbot modify labels: stable-nominated T-compiler |
We discussed this in the T-compiler meeting today. @nagisa says they would prefer we not take on the risk of backporting this to the stable channel; given the state of the code they started with and the dificulty in converging on the version that landed, they are not sure the patch is rock solid. Declining backport to stable branch. |
I understand this is likely difficult to backport, however I find the decision unfortunate, as it effectively breaks the ability to run Are peripheral tools such as In the future, we may want to expand the number of items tested in the |
The changes should already be on beta, can you test? |
Yes, this is fixed on the current beta. |
FWIW I backported this locally without issue (the touched files are only changed rarely), but since there's no information on how to build the thumb libcore I couldn't properly test it. We could also do a crater run with this on stable, I imagine? |
Yes, works for me in beta. |
I do not know the process, but would it be possible to do a point release for a single target? |
@estebank not really, since the thumb libcore's are distributed as separate rustup components. They plug into the regular stable toolchain that is already installed - you don't need a new rustc to target them. |
The questions above posted by @jamesmunns seem like they need to be addressed by ... the @rust-lang/dev-tools team? Or @rust-lang/core ?
|
Re. |
Just for the record: It's not a |
Sort of, those targets are tier 2, so they're guaranteed to build but not guaranteed to work. |
There was a discussion at the 2018 all hands about making the thumb targets tier 1, however this was not done as @alexchrichton mentioned that the concept of tiers was going away, or was being retooled. If not, I'd like to reintroduce the discussion of making the thumb targets an official tier one target, as it already has some of the ground work laid to claim this |
You want @rust-lang/cargo Clippy here just uses cargo check under the hood. I do think that we shouldn't break cargo check for embedded users as much as possible. |
Since there is nothing to optimise there...
Should fix #58323 but haven’t tested locally.
r? @michaelwoerister