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

Issue 57000043: code review 57000043: liblink,cmd/5l: Fix flag_shared (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 5 months ago by elias.naur
Modified:
11 years, 4 months ago
Reviewers:
gobot, rsc, dave, iant
CC:
iant, golang-codereviews
Visibility:
Public.

Description

liblink, cmd/5l: restore flag_shared CL 56120043 fixed and cleaned up TLS on ARM after introducing liblink, but left flag_shared broken. This CL restores the (unsupported) flag_shared behaviour by simply rewriting access to $runtime.tlsgm(SB) with runtime.tlsgm(SB), to compensate for the extra indirection when going from the R_ARM_TLS_LE32 relocation to the R_ARM_TLS_IE32 relocation. Also, remove unnecessary symbol lookup left after 56120043.

Patch Set 1 #

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

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

Total comments: 1

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -6 lines) Patch
M src/cmd/5l/5.out.h View 1 1 chunk +1 line, -1 line 0 comments Download
M src/liblink/asm5.c View 1 1 chunk +1 line, -2 lines 0 comments Download
M src/liblink/obj5.c View 1 2 3 3 chunks +15 lines, -3 lines 0 comments Download

Messages

Total messages: 11
elias.naur
Hello iant@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
11 years, 5 months ago (2014-01-25 16:49:19 UTC) #1
iant
https://codereview.appspot.com/57000043/diff/40001/src/liblink/obj5.c File src/liblink/obj5.c (right): https://codereview.appspot.com/57000043/diff/40001/src/liblink/obj5.c#newcode160 src/liblink/obj5.c:160: if(p->from.name == D_EXTERN && p->from.sym == ctxt->gmsym) I would ...
11 years, 5 months ago (2014-02-03 19:29:40 UTC) #2
elias.naur
On 2014/02/03 19:29:40, iant wrote: > https://codereview.appspot.com/57000043/diff/40001/src/liblink/obj5.c > File src/liblink/obj5.c (right): > > https://codereview.appspot.com/57000043/diff/40001/src/liblink/obj5.c#newcode160 > ...
11 years, 5 months ago (2014-02-03 21:54:55 UTC) #3
elias.naur
PTAL (sorry for the spam, the previous PTAL didn't register on the go-dev dashboard) On ...
11 years, 5 months ago (2014-02-03 22:33:16 UTC) #4
iant
LGTM
11 years, 5 months ago (2014-02-03 22:49:50 UTC) #5
iant
*** Submitted as https://code.google.com/p/go/source/detail?r=1fda0c956e3f *** liblink, cmd/5l: restore flag_shared CL 56120043 fixed and cleaned up ...
11 years, 5 months ago (2014-02-03 22:50:00 UTC) #6
dave_cheney.net
Elias, thanks for working on this. On Tue, Feb 4, 2014 at 9:50 AM, <iant@golang.org> ...
11 years, 5 months ago (2014-02-03 22:53:58 UTC) #7
gobot
This CL appears to have broken the netbsd-386-minux builder.
11 years, 5 months ago (2014-02-03 23:00:55 UTC) #8
rsc
Thanks for working on this, but I am not sure this is the right approach. ...
11 years, 4 months ago (2014-02-10 14:48:42 UTC) #9
elias.naur
On 2014/02/10 14:48:42, rsc wrote: > Thanks for working on this, but I am not ...
11 years, 4 months ago (2014-02-10 16:17:12 UTC) #10
rsc
11 years, 4 months ago (2014-02-24 15:33:03 UTC) #11
On Mon, Feb 10, 2014 at 11:17 AM, <elias.naur@gmail.com> wrote:

> Perhaps this particular code transformation belongs in the linker, and
> I'll gladly work on that if needed. However, I don't think flag_shared
> (or equivalent) can be entirely removed from liblink without sacrificing
> performance for regular Go programs. For example, instructions of the
> form PUSHQ $<sym>(SB) in PC relative mode needs a temporary register
> (and less importantly for this discussion, an extra instruction). On
> ARM, we have the luxury of the link scratch register for rewrites, but
> on amd64 (and probably also 386), we don't have that luxury.
>

you're right, i was thinking of -linkmode.

>
Sign in to reply to this message.

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