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

Target stack-probe support configurable finely #80838

Merged
merged 2 commits into from
Jan 24, 2021
Merged

Target stack-probe support configurable finely #80838

merged 2 commits into from
Jan 24, 2021

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jan 9, 2021

This adds capability to configure the target's stack probe support in a
more precise manner than just on/off. In particular now we allow
choosing between always inline-asm, always call or either one of those
depending on the LLVM version.

Note that this removes the ability to turn off the generation of the
stack-probe attribute. This is valid to replace it with inline-asm for all targets because
probe-stack="inline-asm" will not generate any machine code on targets
that do not currently support stack probes. This makes support for stack
probes on targets that don't have any right now automatic with LLVM
upgrades in the future.

(This is valid to do based on the fact that clang unconditionally sets
this attribute when -fstack-clash-protection is used, AFAICT)

cc #77885
r? @cuviper

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2021
@bors
Copy link
Contributor

bors commented Jan 13, 2021

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

@cuviper
Copy link
Member

cuviper commented Jan 13, 2021

This makes support for stack probes on targets that don't have
any right now automatic with LLVM upgrades in the future.

I'm a little worried about doing so, especially since other targets are tier 2 or below, untested. We're already gating the x86 version based on known bugs, but you haven't left any room for InlineOrNone to be filtered.

(This is valid to do based on the fact that clang unconditionally sets
this attribute when -fstack-clash-protection is used, AFAICT)

I'm not sure clang is correct here. At least on Windows, X86TargetLowering::getStackProbeSymbolName is supposed to fall through and choose the ABI __chkstk, but it looks like that will use the "probe-stack" attribute when set. Maybe this means that you just shouldn't use -fstack-clash-protection at all on Windows, but then we shouldn't set the attribute either.

@nagisa
Copy link
Member Author

nagisa commented Jan 14, 2021

We're already gating the x86 version based on known bugs, but you haven't left any room for InlineOrNone to be filtered.

I was torn between making this PR provide exhaustive set of options for various future use-cases, anticipated or not, or just the fairly minimal set of options that are useful today, with an anticipation that it would be fleshed out as new ones come up.

I'm not sure clang is correct here.

Okay, yeah fair. I'll just add another None option and make it the default for now.

@bors
Copy link
Contributor

bors commented Jan 16, 2021

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

@nagisa
Target stack-probe support configurable finely
007080b 
This adds capability to configure the target's stack probe support in a
more precise manner than just on/off. In particular now we allow
choosing between always inline-asm, always call or either one of those
depending on the LLVM version on a per-target basis.
@nagisa
Copy link
Member Author

nagisa commented Jan 19, 2021

@cuviper fyi I did adjust the PR.

@nagisa nagisa mentioned this pull request Jan 19, 2021
Closed
@bjorn3 bjorn3 mentioned this pull request Jan 20, 2021
Closed
@nagisa nagisa mentioned this pull request Jan 20, 2021
Open
@adamrk adamrk mentioned this pull request Jan 20, 2021
Closed
@cuviper
Copy link
Member

cuviper commented Jan 23, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2021

📌 Commit 1b15ec6 has been approved by cuviper

@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 Jan 23, 2021
@bors
Copy link
Contributor

bors commented Jan 23, 2021

⌛ Testing commit 1b15ec6 with merge 8d234f8b7cfac73bfca027ae1c4f92be933642c3...

@bors
Copy link
Contributor

bors commented Jan 23, 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 Jan 23, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v2'
##[warning]Failed to download action 'https://api.github.com/repos/actions/checkout/zipball/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f'. Error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
##[warning]Back off 28.554 seconds before retry.
##[warning]Failed to download action 'https://api.github.com/repos/actions/checkout/zipball/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f'. Error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
##[warning]Back off 29.198 seconds before retry.
##[error]A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.

@bjorn3
Copy link
Member

bjorn3 commented Jan 23, 2021

@bors retry Spurious network error

@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 Jan 23, 2021
@bors
Copy link
Contributor

bors commented Jan 24, 2021

⌛ Testing commit 1b15ec6 with merge 72c7b70...

@bors
Copy link
Contributor

bors commented Jan 24, 2021

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 72c7b70 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 24, 2021
@bors bors merged commit 72c7b70 into rust-lang:master Jan 24, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 24, 2021
@bors bors mentioned this pull request Jan 24, 2021
Merged
@pnkfelix pnkfelix mentioned this pull request Mar 23, 2021
Merged
pnkfelix added a commit to pnkfelix/rust that referenced this pull request Mar 23, 2021
@pnkfelix
On stable, probe-stack=call everywhere again, for now.
1aa1785 
To buy time on issue 83139, revert effect of PR 77885: We will not conditionally
enable probe-stack=inline-asm on LLVM 11+ anymore on any of our targets that
opted into doing so on PR rust-lang#77885 (and were subsequently configured to do so in a
fine grained manner on PR rust-lang#80838).

After we resolve 83139 (potentially by backporting a fix to LLVM, or potentially
by deciding that one cannot rely on the quality of our DWARF output in the
manner described in issue 83139), we can change this back.

(Update: fixed formatting issue.)
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 23, 2021
@bors
Auto merge of rust-lang#83412 - pnkfelix:stable-revert-effect-of-pr-7…
2fd73fa 
…7885-for-issue-83139, r=nagisa

[stable] probe-stack=call everywhere again, for now.

To buy time on issue 83139, revert effect of PR 77885: We will not conditionally
enable probe-stack=inline-asm on LLVM 11+ anymore on any of our targets that
opted into doing so on PR rust-lang#77885 (and were subsequently configured to do so in a
fine grained manner on PR rust-lang#80838).

After we resolve 83139 (potentially by backporting a fix to LLVM, or potentially
by deciding that one cannot rely on the quality of our DWARF output in the
manner described in issue 83139), we can change this back.

cc rust-lang#83139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper left review comments

Assignees

@cuviper cuviper

Labels
merged-by-bors This PR was explicitly merged by bors. 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.51.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nagisa @rust-highfive @bors @cuviper @rust-log-analyzer @bjorn3 @rustbot