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

all: avoid clobbering BP register #156

Closed
mmcloughlin opened this issue Aug 24, 2020 · 5 comments
Closed

all: avoid clobbering BP register #156

mmcloughlin opened this issue Aug 24, 2020 · 5 comments
Labels
bug Something isn't working

Comments

@mmcloughlin
Copy link
Owner

See discussion on Slack and golang-dev

https://gophers.slack.com/archives/C6WDZJ70S/p1598210629000900
https://groups.google.com/g/golang-dev/c/aLn9t8tKg2o/m/Kw-N7lUuBAAJ

Summary:

  • BP should be callee-save (poorly documented, even some standard library assembly didn't handle this correctly until recently)
  • BP "will be saved automatically if there is a nonzero frame size" https://golang.org/cl/248260

avo needs to handle this somehow. Suggestions:

@mmcloughlin mmcloughlin added the bug Something isn't working label Aug 24, 2020
twmb added a commit to twmb/murmur3 that referenced this issue Aug 25, 2020
ref mmcloughlin/avo#156
which contains links to other relevant refs
@armfazh
Copy link

armfazh commented Mar 2, 2021

If during code generation, avo needs to use BP then it must be saved, which in turn requires at least 8 bytes in the stack.
So this is not possible: someone needs a 0 stack frame and to use BP

De-prioritize BP in the register allocator Yes definitely, BP can reside in the bottom to the list used for register assignment.
A more drastic alternative is not to use BP at all, but I think a push&pop operation is not so bad.

@mmcloughlin
Copy link
Owner Author

Just to test the simple solution, I tried marking BP restricted so it would be excluded from allocation #173. This caused some avo examples to fail, and broke one of the third-party tests.

Therefore, I think the more nuanced solution is worth pursuing, specifically de-prioritizing BP in the allocator, and then applying the "non-zero frame size hack" in the event that the allocator chooses to use BP.

@Yawning
Copy link

Yawning commented Apr 16, 2021

First of all, thanks for avo, it has been useful and helped me make what formerly was a gigantic collection of .s files much easier to maintain.

For what it's worth, the way I am working around this problem for now is to use a nasty kludge involving reflection and unsafe to mark BP as restricted, since I know that I do not need to use rbp as a GP register, and the only thing causing issues is the register allocator.

While this case should be addressed by the allocator gaining a notion of priority, it would be nice for me to continue to take this general approach (preferably without unsafe/reflection).

My current thinking is that in the event that I write something that actually needs every single register in the future after avo starts applying the non-zero frame size hack, I would prefer to handle allocating/saving/restoring rbp myself.

Some minor questions regarding the "non-zero frame size" hack:

  • If the routine handles saving/restoring rbp, will avo not apply the hack?
  • In the event that the hack is required, and the routine has the NOFRAME attribute, what happens?

mmcloughlin added a commit that referenced this issue Apr 19, 2021
Currently `avo` uses `BP` as a standard general-purpose register. However, `BP` is used for the frame pointer and should be callee-save. Under some circumstances, the Go assembler will do this automatically, but not always. At the moment `avo` can produce code that clobbers the `BP` register. Since Go 1.16 this code will also fail a new `go vet` check.

This PR provides a (currently sub-optimal) fix for the issue. It introduces an `EnsureBasePointerCalleeSaved` pass which will check if the base pointer is written to by a function, and if so will artificially ensure that the function has a non-zero frame size. This will trigger the Go assembler to automatically save and restore the BP register.

In addition, we update the `asmdecl` tool to `asmvet`, which includes the `framepointer` vet check.

Updates #156
@mmcloughlin
Copy link
Owner Author

I have landed #174 and #184 which should de-prioritize BP in register allocation, and apply the "frame size hack" if it must be used. I believe this fixes the immediate problem at hand, so I will close this issue, but that doesn't mean there's no more work to be done related to this.

@Yawning Thanks for your comments. I think the answer to your questions is configurable "compilation options", which could be applied globally or per function. In this case, I could imagine two options: one that controls whether BP is available for allocation at all, and another to control whether avo will attempt to automatically save BP. Do you think this would suffice? I've filed #185 for this feature more generally since there are other use cases for configuration like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants