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

runtime: use register instead of TLS for G pointer for darwin/arm and darwin/arm64 #24793

Open
randall77 opened this issue Apr 10, 2018 · 9 comments
Labels
Milestone

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Apr 10, 2018

Need to decide how to handle TLS on darwin/arm. On x86, TLS was moved from a pthread_get/setspecific implementation to a reserved (by Apple) slot in the TLS (see #23617). That's not possible for arm/arm64 so we still use the old method. Since these archs have more registers, we should probably just reserve a register for the G pointer.

Reserving a register might invalidate some existing assembly code. Kind of a wrench in the plan, need to investigate how bad it might be.

Or maybe someone can think of a better solution.

We might need a solution before we can do #17490 for arm.

@randall77 randall77 added the OS-Darwin label Apr 10, 2018
@randall77 randall77 added this to the Go1.12 milestone Apr 10, 2018
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 10, 2018

I think we already do reserve a G register on ARM and ARM64, regardless of OS. This is R10 on ARM, and R28 on ARM64. TLS is only used when crossing Go-C boundary, where we save G register to a TLS slot when calling to C, and reload it when entering Go code, as C code may clobber our G register.

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Apr 10, 2018

Maybe this is unnecessary then.

@eliasnaur
Copy link
Contributor

@eliasnaur eliasnaur commented Apr 10, 2018

The pthread_get/set magic is still present in runtime/cgo/gcc_darwin_arm*.c so I wouldn't qualify that as unnecessary.

@cherrymui given that we only use the TLS slot for Cgo, could we use a callee-saved register for G and thus avoid the TLS slot (and the save/restore) altogether?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 10, 2018

I haven't looked into the code in detail, but I'm not sure callee-saved register is enough. C code can still temporarily use the register, and a signal can happen at this point. The signal handler, runtime.sigtramp needs to look at the G register to tell whether/which Go stack we are on.

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 10, 2018

Backing up one level: why does runtime.sigtramp need to know which goroutine is running if we're in a C call?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 11, 2018

I'm not sure I completely understand the question, but see the sigtramp code in runtime/sys_darwin_arm.s and runtime/sys_darwin_arm64.s. It does today examine the g register, which it loads out of the TLS slot.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Apr 11, 2018

Besides signal handler, another place where we need G is when Go calls C which calls back to Go. There we want to use the same G, I think.

@aclements
Copy link
Member

@aclements aclements commented Jan 8, 2019

What's the status of this? Is there actually a bug here?

@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@eliasnaur
Copy link
Contributor

@eliasnaur eliasnaur commented May 4, 2019

#17490 is fixed, even for iOS, so the remaining bug is that we're relying on an undocumented assumption that a TLS slot allocated by pthread_key_create is located at some positive offset from the TLS base. See

err = pthread_key_create(&k, nil);
if(err != 0) {
fprintf(stderr, "runtime/cgo: pthread_key_create failed: %d\n", err);
abort();
}
//fprintf(stderr, "runtime/cgo: k = %d, tlsbase = %p\n", (int)k, tlsbase); // debug
pthread_setspecific(k, (void*)magic);
// The first key should be at 257.
for (i=0; i<PTHREAD_KEYS_MAX; i++) {
if (*(tlsbase+i) == (void*)magic) {
*tlsg = (void*)(i*sizeof(void *));
pthread_setspecific(k, 0);
return;
}
}

We recently had to remove a similar assumption for Android (#29674).

Since we use a register for g on arm and arm64 the appropriate fix for this issue is probably to call pthread_getspecific and pthread_setspecific when entering or exiting C code.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.