Skip to content

bootstrap: Disable initial-exec TLS model on powerpc - #85807

Merged
bors merged 1 commit into
rust-lang:masterfrom
glaubitz:powerpc-disable-initial-exec-tls
Aug 6, 2021
Merged

bootstrap: Disable initial-exec TLS model on powerpc#85807
bors merged 1 commit into
rust-lang:masterfrom
glaubitz:powerpc-disable-initial-exec-tls

Conversation

@glaubitz

@glaubitzglaubitz commented May 29, 2021

Copy link
Copy Markdown
Contributor

Fixes#81334.

@rust-highfive

rust-highfive commented May 29, 2021

Copy link
Copy Markdown
Contributor

r? @Mark-Simulacrum

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2021
@glaubitz

glaubitz commented May 29, 2021

Copy link
Copy Markdown
ContributorAuthor

Please note: I have not fully tested this change yet. So please don't merge it yet although I'm sure it's correct.

mati865
mati865 reviewed May 29, 2021
View reviewed changes
Comment threadsrc/bootstrap/builder.rs Outdated

@mati865mati865May 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think cfg! macro would be nicer here.

@Mark-SimulacrumMark-SimulacrumMay 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, I'm surprised to see cfg here at all; I'd expect this to check target.contains("powerpc") or so...

@infinity0infinity0May 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe cfg would be wrong when cross-compiling - since "target" here refers to the target of bootstrap which would be the host (native) platform?

@infinity0infinity0May 29, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@glaubitz try && !target.triple.startswith("powerpc-") instead

@glaubitzglaubitzMay 31, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done. I'm testing the change now on a real PowerPC machine to see if it actually works.

@glaubitzglaubitzMay 31, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The change as suggested by @infinity0 works fine and fixes the problem on 32-bit PowerPC.

But please feel free to use a different approach if anyone has a better idea. I don't insist on my particular approach.

@Aaron1011

Aaron1011 commented May 29, 2021

Copy link
Copy Markdown
Contributor

If this TLS mode is completely unsupported on powerpc, then I think we should emit an error whenver someone tries to specify it on the command line. Otherwise, anyone else using -Ztls-model will run to this issue. This could be done in a follow-up PR.

@Mark-SimulacrumMark-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 May 29, 2021
@glaubitz
glaubitzforce-pushed the powerpc-disable-initial-exec-tls branch from c4b54f3 to 674e27eCompareMay 31, 2021 08:17
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@glaubitz
glaubitzforce-pushed the powerpc-disable-initial-exec-tls branch from 674e27e to 283619cCompareMay 31, 2021 08:21
@glaubitz

glaubitz commented May 31, 2021

Copy link
Copy Markdown
ContributorAuthor

If this TLS mode is completely unsupported on powerpc, then I think we should emit an error whenver someone tries to specify it on the command line. Otherwise, anyone else using -Ztls-model will run to this issue. This could be done in a follow-up PR.

Is there some sort of feature matrix that Rust is carrying internally for every architecture where this information could be stored as a flag? I would guess this bug could affect the upcoming m68k port as well.

Plus, such a feature matrix could be used to determine whether a certain test is run or not. Currently, all tests seem to be checking per architecture rather than per feature.

@petrochenkov

petrochenkov commented May 31, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

If this TLS mode is completely unsupported on powerpc, then I think we should emit an error whenver someone tries to specify it on the command line.

This mode is an optimization hint, if an "initial-exec" hint is useless for a given target (like e.g. TLS models not generally mapping on Windows-based target), then it should be demoted to a weaker hint.
If initial-exec not being supported on powerpc is just some LLVM bug, then it can be demoted to a weaker mode as well, until the bug is fixed.
I'm not sure why LLVM doesn't currently reset initial-exec TLS mode to something acceptable on powerpc, it does that for other targets.

@joelpalmerjoelpalmer 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2021
@crlf0710

crlf0710 commented Jul 4, 2021

Copy link
Copy Markdown
Member

@glaubitz Ping from triage, what's next steps here?

@bstrie

bstrie commented Jul 21, 2021

Copy link
Copy Markdown
Contributor

@glaubitz Are you intending to add anything more to this PR, or shall I tag this for review?

@glaubitz

glaubitz commented Jul 22, 2021

Copy link
Copy Markdown
ContributorAuthor

@bkaestner I don't have any strong preference, the fix works for me. Feel free to merge it as-is or replace it with a better solution.

@bkaestner

bkaestner commented Jul 22, 2021

Copy link
Copy Markdown
Contributor

@bkaestner I don't have any strong preference, the fix works for me. Feel free to merge it as-is or replace it with a better solution.

@bstrie I believe this message was meant for you.

@glaubitz

glaubitz commented Jul 22, 2021

Copy link
Copy Markdown
ContributorAuthor

Oops, sorry. Two people starting with "@b" in the thread and I just had only one coffee this morning :).

@glaubitz

glaubitz commented Jul 31, 2021

Copy link
Copy Markdown
ContributorAuthor

@glaubitz Are you intending to add anything more to this PR, or shall I tag this for review?

I don't want to add anything else. Could you tag this for review? Thanks!

@glaubitz

glaubitz commented Aug 4, 2021

Copy link
Copy Markdown
ContributorAuthor

r? @Mark-Simulacrum

@bstriebstrie 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 Aug 4, 2021
@Mark-SimulacrumMark-Simulacrum mentioned this pull request Aug 6, 2021
Open
@Mark-Simulacrum

Mark-Simulacrum commented Aug 6, 2021

Copy link
Copy Markdown
Member

Filed #87821 to track the automatic reduction or error, but for now let's @bors r+

@bors

bors commented Aug 6, 2021

Copy link
Copy Markdown
Collaborator

📌 Commit 283619c has been approved by Mark-Simulacrum

@borsbors 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 Aug 6, 2021
@JohnTitorJohnTitor mentioned this pull request Aug 6, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 6, 2021
@bors
Auto merge ofrust-lang#87822- JohnTitor:rollup-kxojii0, r=JohnTitor
5ad7389
Rollup of 7 pull requests
Successful merges:
- rust-lang#85807 (bootstrap: Disable initial-exec TLS model on powerpc)
- rust-lang#87761 (Fix overflow in rustc happening if the `err_count()` is reduced in a stage.)
- rust-lang#87775 (Add hint for unresolved associated trait items if the trait has a single item)
- rust-lang#87779 (Remove special case for statement `NodeId` assignment)
- rust-lang#87787 (Use `C-unwind` ABI for `__rust_start_panic` in `panic_abort`)
- rust-lang#87809 (Fix typo in the ptr documentation)
- rust-lang#87816 (Sync rustc_codegen_cranelift)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 352ad62 into rust-lang:masterAug 6, 2021
@rustbotrustbot added this to the 1.56.0 milestone Aug 6, 2021
@glaubitzglaubitz mentioned this pull request Sep 29, 2021
Closed
@Mark-SimulacrumMark-Simulacrum mentioned this pull request Oct 13, 2021
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mati865mati865mati865 left review comments
@Mark-SimulacrumMark-SimulacrumMark-Simulacrum left review comments
+1 more reviewer
@infinity0infinity0infinity0 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@Mark-SimulacrumMark-Simulacrum

Labels

S-waiting-on-borsStatus: 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.

Many tests failing with "Couldn't compile the test" on 32-bit PowerPC with 1.49 beta

14 participants

@glaubitz@rust-highfive@Aaron1011@rust-log-analyzer@petrochenkov@crlf0710@bstrie@bkaestner@Mark-Simulacrum@bors@infinity0@mati865@joelpalmer@rustbot