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

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:
5 years, 9 months ago by crawshaw
Modified:
5 years, 9 months 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
5 years, 9 months 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 ...
5 years, 9 months 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: ...
5 years, 9 months 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 ...
5 years, 9 months 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) ...
5 years, 9 months 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: ...
5 years, 9 months 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, ...
5 years, 9 months ago (2014-07-02 17:41:42 UTC) #7
minux
LGTM.
5 years, 9 months 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. ...
5 years, 9 months ago (2014-07-02 19:06:26 UTC) #9
josharian
> Thanks! I'll wait a bit before submitting, in case anyone else wants > to ...
5 years, 9 months 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 ...
5 years, 9 months 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
5 years, 9 months ago (2014-07-03 20:24:30 UTC) #12
crawshaw
5 years, 9 months 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