Skip to content

cmd/link, runtime: reserve 48 bytes in stack frame for ppc64 #31738

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

Closed
wants to merge 2 commits into from

Conversation

zte-majiang
Copy link
Contributor

C functions requires 48 bytes of space on caller stack frame for ppc64. Without this fix, a simple cgo program(build with CGO_CFLAGS="-O0") could mess the stack and finally trigger a segmentation fault.

Updates #13192

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. label Apr 29, 2019
@laboger
Copy link
Contributor

laboger commented Apr 29, 2019

Should this be linux only? @Helflym

@Helflym
Copy link
Contributor

Helflym commented Apr 29, 2019

I don't think you need to change the whole Go stack. The Go stack is relatively independent from the C stack as far as I know. You should be able to directly use cgoCalleeStackSize and asmcgocallSaveOffset constantes as it's done for AIX.
Changing the FIXED_FRAME seems a bit risky and if it's needed anyway, I'll have to check on AIX. But I'm not sure it will work.
@zte-majiang could you try without changing FIXED_FRAME and keep us updated ?

@zte-majiang
Copy link
Contributor Author

@Helflym Thanks for the advice. I have tried the plan(only adjust the parts of go stack) some days ago, it seemed not working.
A quick search show that there are more than 3 pieces of code that try to set the minimum frame size. So I gave up the plan (which adjust go stack only), and turn to make sure the minimum frame size is always 48 on both side.
In my opinion, it's more clear to keep C and GO stack frames consistent. Or else, there will be two minimum frame sizes, one for GO which is 32, and one for C which is 48. This might make things complex(for example, stack backtrace? I'm not sure).
I will have a try as you suggested tomorrow.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. and removed cla: no Used by googlebot to label PRs as having an invalid CLA. The text of this label should not change. labels Apr 29, 2019
@gopherbot
Copy link
Contributor

This PR (HEAD: 3b6ffd0) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/174317 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Lynn Boger:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Lynn Boger:

Patch Set 1:

(4 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Zhang:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Clément Chigot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: 2a9abfc) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/174317 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Jiang Ma:

Patch Set 1:

(5 comments)

Dear Reviewers:
Thanks for the advices. I have adjusted the patch as you suggested.
Please take a look at the new one. It is ok now?


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Lynn Boger:

Patch Set 2:

Did you run all.bash on this patch? I tried this on a power8 linux/ppc64 RHEL 7 and got failures.


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Lynn Boger:

Patch Set 2:

Patch Set 2:

Did you run all.bash on this patch? I tried this on a power8 linux/ppc64 RHEL 7 and got failures.

Clarification: I ran all.bash against patchset 1 and there were failures. It looks like several changes from patchset 1 were removed in patchset 2. Was that your intention?


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jiang Ma:

Patch Set 2:

Patch Set 2:

Patch Set 2:

Did you run all.bash on this patch? I tried this on a power8 linux/ppc64 RHEL 7 and got failures.

Clarification: I ran all.bash against patchset 1 and there were failures. It looks like several changes from patchset 1 were removed in patchset 2. Was that your intention?
Yes, Changes about adjusting golang frames were removed as suggested.
Could you try patchset 2? I can not run all.bash as I have no power8 machines...
I ran make.bash(and my cgo test program with the help of the user model qemu) successfully on a linux/amd64 ubuntu18.04.


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot gopherbot force-pushed the master branch 3 times, most recently from a1b0af9 to 93a79bb Compare October 1, 2019 22:09
@gopherbot gopherbot force-pushed the master branch 3 times, most recently from 4a7ed1f to 0f992b9 Compare November 5, 2019 02:56
@gopherbot
Copy link
Contributor

Message from Carlos Eduardo Seo:

Patch Set 2:

Could you please rebase this against master? I could run a test on a POWER8 BE system.


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Cherry Mui:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/174317.
After addressing review feedback, remember to publish your drafts!

@heschi heschi closed this Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants