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

Issue 56120043: code review 56120043: liblink, runtime: fix cgo on arm (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 1 month ago by rsc
Modified:
11 years, 1 month ago
Reviewers:
dave, iant
CC:
iant, dave_cheney.net, golang-codereviews
Visibility:
Public.

Description

liblink, runtime: fix cgo on arm The addition of TLS to ARM rewrote the MRC instruction differently depending on whether we were using internal or external linking mode. That's clearly not okay, since we don't know that during compilation, which is when we now generate the code. Also, because the change did not introduce a real MRC instruction but instead just macro-expanded it in the assembler, liblink is rewriting a WORD instruction that may actually be looking for that specific constant, which would lead to very unexpected results. It was also using one value that happened to be 8 where a different value that also happened to be 8 belonged. So the code was correct for those values but not correct in general, and very confusing. Throw it all away. Replace with the following. There is a linker-provided symbol runtime.tlsgm with a value (address) set to the offset from the hardware-provided TLS base register to the g and m storage. Any reference to that name emits an appropriate TLS relocation to be resolved by either the internal linker or the external linker, depending on the link mode. The relocation has exactly the semantics of the R_ARM_TLS_LE32 relocation, which is what the external linker provides. This symbol is only used in two routines, runtime.load_gm and runtime.save_gm. In both cases it is now used like this: MRC 15, 0, R0, C13, C0, 3 // fetch TLS base pointer MOVW $runtime·tlsgm(SB), R2 ADD R2, R0 // now R0 points at thread-local g+m storage It is likely that this change breaks the generation of shared libraries on ARM, because the MOVW needs to be rewritten to use the global offset table and a different relocation type. But let's get the supported functionality working again before we worry about unsupported functionality.

Patch Set 1 #

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

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

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

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

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

Patch Set 7 : diff -r f9e8a970798c https://code.google.com/p/go/ #

Patch Set 8 : diff -r f9e8a970798c https://code.google.com/p/go/ #

Total comments: 3

Patch Set 9 : diff -r 847f3d4e7104 https://code.google.com/p/go/ #

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -98 lines) Patch
src/cmd/ld/data.c View 1 2 3 4 2 chunks +18 lines, -7 lines 0 comments Download
src/cmd/ld/lib.c View 1 2 3 4 1 chunk +1 line, -4 lines 0 comments Download
src/liblink/asm5.c View 1 2 3 4 5 1 chunk +9 lines, -1 line 0 comments Download
src/liblink/obj5.c View 1 2 3 4 5 6 7 8 3 chunks +0 lines, -74 lines 0 comments Download
src/pkg/runtime/asm_arm.s View 1 2 3 4 5 6 7 1 chunk +23 lines, -12 lines 0 comments Download

Messages

Total messages: 7
rsc
Hello iant (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
11 years, 1 month ago (2014-01-23 20:04:44 UTC) #1
dave_cheney.net
On 2014/01/23 20:04:44, rsc wrote: > Hello iant (cc: mailto:golang-codereviews@googlegroups.com), > > I'd like you ...
11 years, 1 month ago (2014-01-23 20:51:10 UTC) #2
dave_cheney.net
On 2014/01/23 20:51:10, dfc wrote: > On 2014/01/23 20:04:44, rsc wrote: > > Hello iant ...
11 years, 1 month ago (2014-01-23 20:51:31 UTC) #3
dave_cheney.net
LGTM. Thanks for fixing this Russ. ./all.bash now passes on my chromebook, I'll test on ...
11 years, 1 month ago (2014-01-23 21:19:06 UTC) #4
rsc
Thanks for the unused warnings. That's actually unfortunate, but so be it. It's going to ...
11 years, 1 month ago (2014-01-24 00:01:06 UTC) #5
iant
LGTM https://codereview.appspot.com/56120043/diff/140001/src/cmd/ld/data.c File src/cmd/ld/data.c (right): https://codereview.appspot.com/56120043/diff/140001/src/cmd/ld/data.c#newcode174 src/cmd/ld/data.c:174: // This 8 seems to be a fundamental ...
11 years, 1 month ago (2014-01-24 00:18:17 UTC) #6
rsc
11 years, 1 month ago (2014-01-24 03:51:44 UTC) #7
*** Submitted as https://code.google.com/p/go/source/detail?r=6ef642823263 ***

liblink, runtime: fix cgo on arm

The addition of TLS to ARM rewrote the MRC instruction
differently depending on whether we were using internal
or external linking mode. That's clearly not okay, since we
don't know that during compilation, which is when we now
generate the code. Also, because the change did not introduce
a real MRC instruction but instead just macro-expanded it
in the assembler, liblink is rewriting a WORD instruction that
may actually be looking for that specific constant, which would
lead to very unexpected results. It was also using one value
that happened to be 8 where a different value that also
happened to be 8 belonged. So the code was correct for those
values but not correct in general, and very confusing.

Throw it all away.

Replace with the following. There is a linker-provided symbol
runtime.tlsgm with a value (address) set to the offset from the
hardware-provided TLS base register to the g and m storage.
Any reference to that name emits an appropriate TLS relocation
to be resolved by either the internal linker or the external linker,
depending on the link mode. The relocation has exactly the
semantics of the R_ARM_TLS_LE32 relocation, which is what
the external linker provides.

This symbol is only used in two routines, runtime.load_gm and
runtime.save_gm. In both cases it is now used like this:

        MRC		15, 0, R0, C13, C0, 3 // fetch TLS base pointer
        MOVW	$runtime·tlsgm(SB), R2
        ADD	R2, R0 // now R0 points at thread-local g+m storage

It is likely that this change breaks the generation of shared libraries
on ARM, because the MOVW needs to be rewritten to use the global
offset table and a different relocation type. But let's get the supported
functionality working again before we worry about unsupported
functionality.

LGTM=dave, iant
R=iant, dave
CC=golang-codereviews
https://codereview.appspot.com/56120043
Sign in to reply to this message.

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