Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(600)

Issue 106380043: code review 106380043: cmd/go, cmd/ld, runtime, os/user: TLS emultion for android (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years ago by crawshaw
Modified:
6 years ago
Reviewers:
gobot, minux
CC:
elias.naur, minux, dfc, josharian, golang-codereviews
Visibility:
Public.

Description

cmd/go, cmd/ld, runtime, os/user: TLS emulation for android Based on cl/69170045 by Elias Naur. There are currently several schemes for acquiring a TLS slot to save the g register. None of them appear to work for android. The closest are linux and darwin. Linux uses a linker TLS relocation. This is not supported by the android linker. Darwin uses a fixed offset, and calls pthread_key_create until it gets the slot it wants. As the runtime loads late in the android process lifecycle, after an arbitrary number of other libraries, we cannot rely on any particular slot being available. So we call pthread_key_create, take the first slot we are given, and put it in runtime.tlsg, which we turn into a regular variable in cmd/ld. Makes android/arm cgo binaries work.

Patch Set 1 #

Patch Set 2 : diff -r 86b431eeb5c2 https://code.google.com/p/go #

Patch Set 3 : diff -r 86b431eeb5c2 https://code.google.com/p/go #

Total comments: 38

Patch Set 4 : diff -r 86b431eeb5c2 https://code.google.com/p/go #

Total comments: 4

Patch Set 5 : diff -r 0f766178488c https://code.google.com/p/go #

Patch Set 6 : diff -r c7f38353757f https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+153 lines, -65 lines) Patch
M src/cmd/go/build.go View 1 2 3 1 chunk +3 lines, -2 lines 0 comments Download
M src/cmd/ld/data.c View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M src/cmd/ld/elf.c View 1 1 chunk +2 lines, -1 line 0 comments Download
M src/cmd/ld/lib.c View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M src/cmd/ld/pobj.c View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/ld/symtab.c View 1 1 chunk +6 lines, -1 line 0 comments Download
M src/pkg/os/user/lookup_stubs.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/os/user/lookup_unix.go View 1 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/runtime/asm_arm.s View 1 2 3 2 chunks +6 lines, -40 lines 0 comments Download
M src/pkg/runtime/cgo/cgo.go View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
A src/pkg/runtime/cgo/gcc_android_arm.c View 1 2 3 1 chunk +48 lines, -0 lines 0 comments Download
M src/pkg/runtime/cgo/gcc_linux_arm.c View 1 2 3 4 5 3 chunks +21 lines, -17 lines 0 comments Download
A src/pkg/runtime/tls_arm.s View 1 2 3 4 1 chunk +54 lines, -0 lines 0 comments Download

Messages

Total messages: 13
crawshaw
Hello elias.naur@gmail.com, minux@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
6 years ago (2014-07-02 02:59:38 UTC) #1
minux
https://codereview.appspot.com/106380043/diff/40001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/106380043/diff/40001/src/cmd/go/build.go#newcode1745 src/cmd/go/build.go:1745: compiler = append(compiler, "-llog") put -llog as a #cgo ...
6 years ago (2014-07-02 04:21:15 UTC) #2
dfc
https://codereview.appspot.com/106380043/diff/40001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/106380043/diff/40001/src/cmd/go/build.go#newcode1745 src/cmd/go/build.go:1745: compiler = append(compiler, "-llog") On 2014/07/02 04:21:14, minux wrote: ...
6 years ago (2014-07-02 04:33:05 UTC) #3
crawshaw
Thank you both for the careful review. PTAL. https://codereview.appspot.com/106380043/diff/40001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/106380043/diff/40001/src/cmd/go/build.go#newcode1745 src/cmd/go/build.go:1745: compiler ...
6 years ago (2014-07-02 14:57:22 UTC) #4
elias.naur
https://codereview.appspot.com/106380043/diff/60001/src/cmd/ld/data.c File src/cmd/ld/data.c (right): https://codereview.appspot.com/106380043/diff/60001/src/cmd/ld/data.c#newcode164 src/cmd/ld/data.c:164: if (strcmp(goos, "android") == 0 && r->type == R_TLS) ...
6 years ago (2014-07-02 16:05:59 UTC) #5
minux
https://codereview.appspot.com/106380043/diff/40001/src/cmd/ld/pobj.c File src/cmd/ld/pobj.c (right): https://codereview.appspot.com/106380043/diff/40001/src/cmd/ld/pobj.c#newcode145 src/cmd/ld/pobj.c:145: if(strcmp(goos, "android") == 0) On 2014/07/02 14:57:21, crawshaw wrote: ...
6 years ago (2014-07-02 17:16:54 UTC) #6
crawshaw
https://codereview.appspot.com/106380043/diff/40001/src/pkg/runtime/cgo/gcc_linux_arm.c File src/pkg/runtime/cgo/gcc_linux_arm.c (right): https://codereview.appspot.com/106380043/diff/40001/src/pkg/runtime/cgo/gcc_linux_arm.c#newcode39 src/pkg/runtime/cgo/gcc_linux_arm.c:39: fprintf(stderr, "runtime/cgo: pthread_create failed: %s\n", strerror(err)); On 2014/07/02 17:16:53, ...
6 years ago (2014-07-02 17:41:42 UTC) #7
minux
LGTM.
6 years ago (2014-07-02 18:29:10 UTC) #8
crawshaw
Thanks! I'll wait a bit before submitting, in case anyone else wants to chime in. ...
6 years ago (2014-07-02 19:06:26 UTC) #9
josharian
> Thanks! I'll wait a bit before submitting, in case anyone else wants > to ...
6 years ago (2014-07-02 19:29:18 UTC) #10
crawshaw
*** Submitted as https://code.google.com/p/go/source/detail?r=d234a6fdf1df *** cmd/go, cmd/ld, runtime, os/user: TLS emulation for android Based on ...
6 years ago (2014-07-03 20:14:55 UTC) #11
gobot
This CL appears to have broken the linux-arm-panda builder. See http://build.golang.org/log/618d1c7558673c76439472fec9b44d879f324098
6 years ago (2014-07-03 20:24:30 UTC) #12
crawshaw
6 years ago (2014-07-03 20:26:17 UTC) #13
Real, sorry. CL coming.

On Thu, Jul 3, 2014 at 4:24 PM,  <gobot@golang.org> wrote:
> This CL appears to have broken the linux-arm-panda builder.
> See http://build.golang.org/log/618d1c7558673c76439472fec9b44d879f324098
>
> https://codereview.appspot.com/106380043/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b