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

Make llvm-libunwind a per-target option #93604

Merged
merged 1 commit into from
May 25, 2022
Merged

Make llvm-libunwind a per-target option #93604

merged 1 commit into from
May 25, 2022

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Feb 2, 2022

Fuchsia doesn't ship libunwind in its SDK, so we must provide it statically.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

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

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

cc @12101111

@tmandry tmandry 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2022
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 5, 2022
Mark-Simulacrum
Mark-Simulacrum reviewed Feb 6, 2022
View reviewed changes
config.toml.example Outdated
# means static link to the in-tree build of llvm libunwind, and 'system' means
# static link to `libunwind.a` provided by system. Due to the limitation of glibc,
# it must link to `libgcc_eh.a` to get a working output, and this option have no effect.
# Global default for llvm-libunwind for all targets. Overrides target default.
Copy link
Member

Choose a reason for hiding this comment

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

I would not expect this to override the target default? Unless I'm misunderstanding, I would expect us to have:

  • target.llvm-libunwind (if set)
  • (global) llvm-libunwind (if set)
  • target.llvm-libunwind (default)
  • (global) llvm-unwind (default)

as the rough order.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the intended order. I'm trying to point out the relative ordering between #2 and #3 in your comment. Can you think of wording that would be clearer?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just essentially inlining my list as-is? Maybe saying something like "Note that the target-specific option llvm-libunwind will override this if set" would work too. "Overrides target default" to me feels a little unclear -- I think it can only mean one thing on a second reading, but the initial implication seems confusing, maybe because it is saying something that sounds obviously true to me :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Went with your second suggestion, is this more clear?

@Mark-Simulacrum Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2022
@tmandry tmandry added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2022
@Mark-Simulacrum
Copy link
Member

Yes, seems great.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 10, 2022

📌 Commit 42624ba 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 Mar 10, 2022
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Mar 10, 2022
@m-ou-se
Rollup merge of rust-lang#93604 - tmandry:libunwind-fuchsia-default, …
321e159 
…r=Mark-Simulacrum

Use in-tree libunwind by default on Fuchsia

Fuchsia doesn't ship libunwind in its SDK, so we must provide it statically.
@m-ou-se m-ou-se mentioned this pull request Mar 10, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 10, 2022
@Dylan-DPC
Rollup merge of rust-lang#93604 - tmandry:libunwind-fuchsia-default, …
005d3b7 
…r=Mark-Simulacrum

Use in-tree libunwind by default on Fuchsia

Fuchsia doesn't ship libunwind in its SDK, so we must provide it statically.
@Dylan-DPC Dylan-DPC mentioned this pull request Mar 10, 2022
Closed
@Dylan-DPC
Copy link
Member

@bors r-

failed in rollup

@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 Mar 10, 2022
@bors
Copy link
Contributor

bors commented Mar 13, 2022

☔ The latest upstream changes (presumably #94916) made this pull request unmergeable. Please resolve the merge conflicts.

@apiraino apiraino added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label May 19, 2022
@tmandry tmandry force-pushed the libunwind-fuchsia-default branch from 9d857d9 to f1e3d40 Compare May 24, 2022 23:00
@tmandry tmandry changed the title Use in-tree libunwind by default on Fuchsia Make llvm-libunwind a per-target option May 24, 2022
@tmandry
Copy link
Member Author

tmandry commented May 24, 2022

Since this hit a snag building llvm-libunwind in Rust CI, I've taken out the Fuchsia-specific code from this PR so we can land the per-target logic.

@tmandry
Copy link
Member Author

tmandry commented May 24, 2022

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented May 24, 2022

📌 Commit f1e3d40 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 24, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request May 25, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 25, 2022
@bors
Auto merge of rust-lang#97382 - Dylan-DPC:rollup-2t4ov4z, r=Dylan-DPC
cbdce42 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93604 (Make llvm-libunwind a per-target option)
 - rust-lang#97026 (Change orderings of `Debug` for the Atomic types to `Relaxed`.)
 - rust-lang#97105 (Add tests for lint on type dependent on consts)
 - rust-lang#97323 (Introduce stricter checks for might_permit_raw_init under a debug flag )
 - rust-lang#97379 (Add aliases for `current_dir`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bbb88ea into rust-lang:master May 25, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 25, 2022
@tmandry tmandry deleted the libunwind-fuchsia-default branch May 25, 2022 18:30
@tmandry tmandry mentioned this pull request Jul 28, 2022
Merged
@andrewpollack andrewpollack mentioned this pull request Aug 9, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2022
@bors
Auto merge of rust-lang#100341 - andrewpollack:fuchsia-llvm-libunwind…
e8dc8bc 
…, r=tmandry

Use llvm-libunwind="in-tree" for Fuchsia targets

With updates to Fuchsia CI's Zircon libraries rust-lang#99833, we can introduce `llvm-libunwind="in-tree"` for Fuchsia targets. This PR restores functionality removed from rust-lang#93604 (comment).

cc `@tmandry` `@djkoloski`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.
9 participants
@tmandry @rust-highfive @rust-log-analyzer @petrochenkov @Mark-Simulacrum @bors @Dylan-DPC @apiraino @rustbot