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: android x86 support #9327

Closed
crawshaw opened this issue Dec 15, 2014 · 27 comments
Closed

runtime: android x86 support #9327

crawshaw opened this issue Dec 15, 2014 · 27 comments

Comments

@crawshaw
Copy link
Member

Tracking bug. There are a handful of x86 android devices in the wild, but what makes this target interesting is the Android Emulator. By far the best emulator uses Intel's HAXM kernel plugin to run android via hardware-assisted virtualization.

It also makes it possible to setup an android builder as two docker images (build host and android client) which would be much easier for other's to bring up and maintain.

@jeffallen
Copy link
Contributor

In the meantime, it would be useful to note in the docs that "gomobile install" will only work if you have either real hardware, or an ARM-based AVD. You can download the arm AVD with Android Studio and use it instead of the default x86 one.

@hyangah
Copy link
Contributor

hyangah commented Aug 18, 2015

One obvious missing feature is the TLS handling - linux/386 assumes availability of TLS store to hold g across cgo calls, and the linker's ability to handle TLS relocation for LE mode. But android linker lacks such support.

Android/arm overcomes it by emulating runtime.tlsg as a regular variable. The same trick doesn't seem to work for 386 where the offset is encoded in the instruction when using LE model.

Instead, @crawshaw and I think the approach darwin/386 is taking (runtime/cgo/gcc_darwin_386.c) may work for android/386 as well - basically we allocate a thread-local storage with a specific key and use a hard-coded offset (cmd/link/internal/ld/sym.go line 145). This is ugly and fragile, but seems promising. Should we take this path?

In this mail thread https://groups.google.com/forum/#!topic/golang-nuts/cZPdxDmnuUM @mwhudson mentioned it may be feasible through IE model support and I guess it may be cleaner. However, not being familiar with the concept yet, I cannot estimate how much work is necessary to implement IE model support (pointer to the cl that implements IE mode for amd64 would be appreciated) and not yet clear about where to hook into the tls offset info when the linker is not doing most of the magic for me.

@ianlancetaylor @mwhudson

@mwhudson
Copy link
Contributor

It's probably not that hard to implement IE model for 386 -- i'll need to do it for shared libraries on 386 anyway (a task I keep subconsciously trying to forget!). I don't think this part is much harder than for amd64 (at least, once we handle PIC :/).

@timcooijmans
Copy link
Contributor

https://go-review.googlesource.com/#/c/13760/ adds some missing defines that I encountered while trying to build the std-lib for. android x86.

Another problem is that both src/runtime/cgo/gcc_386.S (from Go) and usr/lib/crtbegin_dynamic.o (from the NDK) define __stack_chk_fail_local. Removing the definition from src/runtime/cgo/gcc_386.S fixes the problem for Android but I could imagine that this breaks other gcc 386 targets.

@gopherbot
Copy link

CL https://golang.org/cl/13615 mentions this issue.

hyangah added a commit that referenced this issue Aug 21, 2015
I cannot find where it's being used.

This addresses a duplicate symbol issue encountered in #9327.

Change-Id: I8efda45a006ad3e19423748210c78bd5831215e0
Reviewed-on: https://go-review.googlesource.com/13615
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@timcooijmans
Copy link
Contributor

I saw the changes by @mwhudson in a5cb762. Is this all that is needed to solve this issue?

@mwhudson
Copy link
Contributor

mwhudson commented Oct 8, 2015

No, there's nothing in that that really changes anything on 386.

On 8 October 2015 at 22:32, timcooijmans notifications@github.com wrote:

I saw the changes by @mwhudson https://github.com/mwhudson in a5cb762
a5cb762.
Is this all that is needed to solve this issue?


Reply to this email directly or view it on GitHub
#9327 (comment).

@rsc
Copy link
Contributor

rsc commented Nov 5, 2015

Is this really Go 1.6? If so it needs to be done very soon. Otherwise it should be moved to the Unplanned milestone. Thanks.

@hyangah
Copy link
Contributor

hyangah commented Nov 5, 2015

I just flushed out a couple of CLs to update this issue but couldn't make the c-shared build mode working yet. Probably I should drop this out of go1.6 milestone.

@gopherbot
Copy link

CL https://golang.org/cl/16678 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/16700 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/16679 mentions this issue.

@hyangah
Copy link
Contributor

hyangah commented Nov 6, 2015

it turned out enabling c-shared was just changing a couple of conditions in flag setting. (cl sent out)
I am afraid my CLs will interfere with mwhudson's 386 work for buildmode=shared support.

@gopherbot
Copy link

CL https://golang.org/cl/16701 mentions this issue.

hyangah added a commit that referenced this issue Nov 11, 2015
Same ugly hack as https://go-review.googlesource.com/15991.

Update #9327.

Change-Id: I58284e83268a15de95eabc833c3e01bf1e3faa2e
Reviewed-on: https://go-review.googlesource.com/16678
Reviewed-by: David Crawshaw <crawshaw@golang.org>
mk0x9 pushed a commit to mk0x9/go that referenced this issue Nov 11, 2015
Update golang#9327.

Change-Id: I27ef973190d9ae652411caf3739414b5d46ca7d2
Reviewed-on: https://go-review.googlesource.com/16679
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@ghost
Copy link

ghost commented Nov 17, 2015

since this depends on shared linking, this will drop as part as go 1.6 ?

hyangah added a commit that referenced this issue Nov 17, 2015
no buildmode=c-shared yet.

Update #9327.

Change-Id: I9989d954d574807bac105da401c3463607fe8a99
Reviewed-on: https://go-review.googlesource.com/16700
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@hyangah
Copy link
Contributor

hyangah commented Nov 18, 2015

as of rev 431c232, android/386 is broken.
I suspect commit 3c85e1b, but not so sure.
GDB revealed a trivial binary crashes at runtime/asm_386.s:550 with SIGSEGV.

Breakpoint 4, runtime.asmcgocall ()
    at /usr/local/google/home/hakim/golang/goroots/386/go/src/runtime/asm_386.s:549
549     MOVL    BX, 0(SP)   // first argument in x86-32 ABI
(gdb) info reg
eax            0xb775ac70   -1217024912
ecx            0xb77385ac   -1217165908
edx            0xbfdcd554   -1076046508
ebx            0xbfdcd578   -1076046472
esp            0xbfdcd530   0xbfdcd530
ebp            0xb77a8e40   0xb77a8e40 
esi            0xb77a8c40   -1216705472
edi            0x5c 92
eip            0xb7758ae4   0xb7758ae4 
eflags         0x216    [ PF AF IF ]
cs             0x73 115
ss             0x7b 123
ds             0x7b 123
es             0x7b 123
fs             0x0  0
gs             0x33 51
(gdb) next
550     CALL    AX
(gdb) next
Program terminated with signal SIGSEGV, Segmentation fault.
(gdb) info symbol 0xb775ac70
x_cgo_thread_start in section .text of /tmp/trivial

@mwhudson

@mwhudson
Copy link
Contributor

I'm sorry I broke this :(

It seems implausible that that CALL AX instruction is the one faulting. For debugging this sort of thing I find the "layout regs" mode of gdb invaluable, and using the si/ni comands to go by one instruction at a time. I'll try to set up the emulator and reproduce this myself, but that's probably going to take a while (I'm also on leave tomorrow).

@griesemer
Copy link
Contributor

@hyangah Just to exclude the (unlikely) possibility that the new parser is screwing things up: Can you run the compiler with the -oldparser flag and see if the problem persists? (e.g. go build -gcflags=-oldparser ).

@mwhudson
Copy link
Contributor

@griesemer I don't think any Go code has been executed by this point :-)

@hyangah
Copy link
Contributor

hyangah commented Nov 19, 2015

@griesemer Thanks! I found this breakage after cherrypicking cl/17012, so I don't think that's related.
@mwhudson Thanks for the tip.

It failed consistently in pthread_create call in _cgo_sys_thread_start (runtime/cgo/gcc_linux_386.c). But seeing it failing while calling _thread_created_hook of pthread_create (I guess it's line 380 of http://osxr.org/android/source/bionic/libc/bionic/pthread.c or equivalent), I am getting skeptical about relying on gdb. :-(

@mwhudson
Copy link
Contributor

Thanks for your testing guide, I got it working, although gdb doesn't seem to want to play, it says:

gdb: Unable to get location for thread creation breakpoint: requested event is not supported
gdb: Unable to get location for thread creation breakpoint: requested event is not supported
gdb: Unable to get location for thread creation breakpoint: requested event is not supported

when I try to do anything. But at least I can reproduce the problem. Are you really truly sure that it's that revision though? I think 3bf61fb is much more likely.

@mwhudson
Copy link
Contributor

Well 3bf61fb fails and the revision before succeeds, so yeah, think it's that (which makes more sense). Back after the weekend.

@hyangah
Copy link
Contributor

hyangah commented Nov 19, 2015

oh, sorry for my initial incorrect report.
Probably, I did something wrong with cherrypicking. Thanks!

@mwhudson
Copy link
Contributor

I couldn't stop thinking about this and pushed a simple fix up for review.

@mwhudson
Copy link
Contributor

I haven't run tests, but a trivial executable runs (and doesn't print warnings about text relocations, woo!)

@gopherbot
Copy link

CL https://golang.org/cl/17049 mentions this issue.

gopherbot pushed a commit that referenced this issue Nov 19, 2015
…/386

golang.org/cl/16383 broke android/386 because by a sort of confluence of hacks
no TLS relocations were emitted at all when Flag_shared != 0. The hack in
runtime/cgo works as well in a PIE executable as it does with a position
dependent one, so the simplest fix is to still emit a R_TLS_LE reloc when goos
== "android".

A real fix is to use something more like the IE model code but loading the
offset from %gs to the thread local storage from a global variable rather than
from a location chosen by the system linker (this is how android/arm works).

Issue #9327.

Change-Id: I9fbfc890ec7fe191f80a595b6cf8e2a1fcbe3034
Reviewed-on: https://go-review.googlesource.com/17049
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
hyangah added a commit that referenced this issue Nov 20, 2015
Update #9327.

Change-Id: Iab7dad31cf6b9f9347c3f34faebb67ecb38b17fc
Reviewed-on: https://go-review.googlesource.com/16701
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@hyangah
Copy link
Contributor

hyangah commented Nov 25, 2015

builder is up.
The way TLS is handled is not ideal but I am closing this bug for now.
Changes to gomobile tool, etc will happen later.

@hyangah hyangah closed this as completed Nov 25, 2015
mwhudson added a commit to mwhudson/go that referenced this issue Jan 4, 2016
…/386

golang.org/cl/16383 broke android/386 because by a sort of confluence of hacks
no TLS relocations were emitted at all when Flag_shared != 0. The hack in
runtime/cgo works as well in a PIE executable as it does with a position
dependent one, so the simplest fix is to still emit a R_TLS_LE reloc when goos
== "android".

A real fix is to use something more like the IE model code but loading the
offset from %gs to the thread local storage from a global variable rather than
from a location chosen by the system linker (this is how android/arm works).

Issue golang#9327.

Change-Id: I9fbfc890ec7fe191f80a595b6cf8e2a1fcbe3034
fsouza pushed a commit to fsouza/go that referenced this issue Feb 25, 2016
no buildmode=c-shared yet.

Update golang/go#9327.

Change-Id: I9989d954d574807bac105da401c3463607fe8a99
Reviewed-on: https://go-review.googlesource.com/16700
Reviewed-by: David Crawshaw <crawshaw@golang.org>
fsouza pushed a commit to fsouza/go that referenced this issue Feb 25, 2016
Update golang/go#9327.

Change-Id: Iab7dad31cf6b9f9347c3f34faebb67ecb38b17fc
Reviewed-on: https://go-review.googlesource.com/16701
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
@golang golang locked and limited conversation to collaborators Nov 27, 2016
@rsc rsc unassigned hyangah Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants