ebitengine / purego

Apache License 2.0
2.16k stars 68 forks source link

SSE instruction segfaults fakecgo init #106

Closed eliottness closed 1 year ago

eliottness commented 1 year ago

Some SSE instructions have requirements that make the fakecgo package segfault. In my case, the call to malloc during x_cgo_init in internal/fakecgo/go_linux_amd64.go makes use of a movaps instruction on 0x10(%rsp). But since %rsp was not aligned on 16 bits (as the instruction requires it), the cgo initialization segfaults. Fixing the register manually using gdb does the trick but of course it messes with the stack and segfault laters.

My team and I are really fond and the project and really want it to work on linux so I will probably drop a PR if I find a fix in the following days. If you have a definitive idea on how to fix this or if you want more debug information, please let me know.

TotallyGamerJet commented 1 year ago

Aligning the stack should be pretty easy. Something likeANDQ $-16, SP. However, I don't understand how the SSE instructions are calling into fakecgo. If you provide a MRE I can experiment with a solution as well. It would also be useful to add as a test.

TotallyGamerJet commented 1 year ago

@eliottness and @klauspost does my branch fix-106 solve this?

eliottness commented 1 year ago

@eliottness and @\klauspost does my branch fix-106 solve this?

@TotallyGamerJet Yes it does for me. Do you still want the MRE I was doing ? This was basically me creating a fake libc.so.6 with something like this:

void *malloc(size_t size)
{
    asm("movaps %rsp, %xmm0");
    return NULL;
}

Then running:

LD_LIBRARY_PATH="$PWD:$LB_LIBRARY_PATH" go test

Not sure how it would integrate with the current testsuite through.

TotallyGamerJet commented 1 year ago

It's not possible to use Cgo in go test. Forcing CGO_ENABLED=0 in the GitHub tests should be enough if it has a similar malloc implementation

eliottness commented 1 year ago

Indeed you are right. I have this action failing on this commit, but this action is fine with this commit. Go ahead and merge what you please. Thanks again for the fix.