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

[beta] Revert c unwind abi PR #84672

Closed
wants to merge 4 commits into from

Conversation

pnkfelix
Copy link
Member

As discussed in #84158 (comment), the compiler team decided we would first try to revert PR #76570 on beta in order to ensure that things like issue #83541 do not hit stable.

This PR is the aforementioned reversion of PR #76570.

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

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

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 28, 2021
@Mark-Simulacrum
Copy link
Member

r=me

@Mark-Simulacrum Mark-Simulacrum changed the title Revert c unwind abi PR [beta] Revert c unwind abi PR Apr 28, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Apr 29, 2021

📌 Commit 6033cca has been approved by Mark-Simulacrum

@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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 29, 2021
@bors
Copy link
Contributor

bors commented Apr 29, 2021

⌛ Testing commit 6033cca with merge 56912a4aca04886881e70a24dc9ba1035bcd417a...

@rust-log-analyzer
Copy link
Collaborator

The job dist-i686-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/i686-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "i686-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/miri/Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json-render-diagnostics"
expected success, got: exit code: 101
thread 'main' panicked at 'Unable to build miri', src/bootstrap/dist.rs:44:9
[TIMING] ToolBuild { compiler: Compiler { stage: 1, host: TargetSelection { triple: "i686-unknown-linux-gnu", file: None } }, target: TargetSelection { triple: "i686-unknown-linux-gnu", file: None }, tool: "miri", path: "src/tools/miri", mode: ToolRustc, is_optional_tool: true, source_type: Submodule, extra_features: [] } -- 2.726
[TIMING] Miri { compiler: Compiler { stage: 1, host: TargetSelection { triple: "i686-unknown-linux-gnu", file: None } }, target: TargetSelection { triple: "i686-unknown-linux-gnu", file: None }, extra_features: [] } -- 0.000
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap dist --build i686-unknown-linux-gnu --host i686-unknown-linux-gnu --target i686-unknown-linux-gnu
Build completed unsuccessfully in 0:32:02

@bors
Copy link
Contributor

bors commented Apr 29, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 29, 2021
@camelid
Copy link
Member

camelid commented Apr 29, 2021

160 errors, I think all from Miri, that look like this:

error[E0559]: variant `rustc_target::spec::abi::Abi::C` has no field named `unwind`
  --> src/tools/miri/src/shims/posix/foreign_items.rs:47:41
   |
47 |                 check_abi(abi, Abi::C { unwind: false })?;
   |                                         ^^^^^^ `rustc_target::spec::abi::Abi::C` does not have this field

@pnkfelix
Copy link
Member Author

closing in favor of PR #84722. (See description of that PR for why; in short, fixing the Miri fallout would have been too painful.)

@pnkfelix pnkfelix closed this Apr 29, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2021
…t-of-84158, r=Mark-Simulacrum

backport: move new c abi abort behavior behind feature gate

This is a backport of PR rust-lang#84158 to the beta branch.

The original T-compiler plan was to revert PR rust-lang#76570 in its entirety, as was attempted in PR rust-lang#84672. But the revert did not go smoothly (details below).

Therefore, we are backporting PR rust-lang#84158 instead, which was our established backup plan if a revert did not go smoothly.

I have manually confirmed that this backport fixes the luajit issue described on issue rust-lang#83541

<details>

<summary>Click for details as to why revert of PR rust-lang#76570 did not go smoothly.</summary>

It turns out that Miri had been subsequently updated to reflect changes to `rustc_target` that landed in PR rust-lang#76570. This meant that the attempt to land PR rust-lang#84672 broke Miri builds.

Normally we allow tools to break when landing PR's (and just expect follow-up PR's to fix the tools), but we don't allow it for tools in the run-up to a release.

(We shouldn't be using that uniform policy for all tools. Miri should be allow to break during the week before a release; but currently we cannot express that, due to issue rust-lang#74709.)

Therefore, its a lot of pain to try to revert PR rust-lang#76570. And we're going with the backup plan.

</details>

Original commit message follows:

----

 *Background*

In rust-lang#76570, new ABI strings including `C-unwind` were introduced. Their behavior is specified in RFC 2945 <sup>[1]</sup>.

However, it was reported in the #ffi-unwind stream of the Rust community Zulip that this had altered the way that `extern "C"` functions behaved even when the `c_unwind` feature gate was not active. <sup>[2]</sup>

 *Overview*

 This makes a small patch to `rustc_mir_build::build::should_abort_on_panic`, so that the same  behavior from before is in place when the `c_unwind` gate is not active.

`rustc_middle::ty::layout::fn_can_unwind` is not touched, as the visible behavior should not differ before/after rust-lang#76570. <sup>[3]</sup>

 ### Footnotes

 1.: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
 2.: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/Is.20unwinding.20through.20extern.20C.20UB.3F/near/230112325
 3.: https://github.com/rust-lang/rust/pull/76570/files#diff-b0320c2b8868f325d83c027fc5d71732636e9763551e35895488f30fe057c6e9L2599-R2617

 [1]: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
 [2]: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/Is.20unwinding.20through.20extern.20C.20UB.3F/near/230112325
 [3]: https://github.com/rust-lang/rust/pull/76570/files#diff-b0320c2b8868f325d83c027fc5d71732636e9763551e35895488f30fe057c6e9L2599-R2617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants