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

Issue 162880044: code review 162880044: undo CL 156430044 / 5d69cad4faaf (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 5 months ago by dfc
Modified:
9 years, 5 months ago
Reviewers:
gobot, minux
CC:
iant, minux, golang-codereviews
Visibility:
Public.

Description

undo CL 156430044 / 5d69cad4faaf Partial undo, changes to ldelf.c retained. Some platforms are still not working even with the integrated assembler disabled, will have to find another solution. ««« original CL description cmd/cgo: disable clang's integrated assembler Fixes issue 8348. Clang's internal assembler (introduced by default in clang 3.4) understands the .arch directive, but doesn't change the default value of -march. This causes the build to fail when we use BLX (armv5 and above) when clang is compiled for the default armv4t architecture (which appears to be the default on all the distros I've used). This is probably a clang bug, so work around it for the time being by disabling the integrated assembler when compiling the cgo assembly shim. This CL also includes a small change to ldelf.c which was required as clang 3.4 and above generate more weird symtab entries. LGTM=iant R=golang-codereviews, iant CC=golang-codereviews https://codereview.appspot.com/156430044 »»»

Patch Set 1 #

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

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

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -7 lines) Patch
M src/cmd/cgo/gcc.go View 1 1 chunk +1 line, -7 lines 0 comments Download

Messages

Total messages: 14
dfc
Hello iant@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
9 years, 5 months ago (2014-10-21 23:20:20 UTC) #1
minux
could you please give more details on how it is not working?
9 years, 5 months ago (2014-10-21 23:24:59 UTC) #2
dfc
On 2014/10/21 23:24:59, minux wrote: > could you please give more details on how it ...
9 years, 5 months ago (2014-10-21 23:25:42 UTC) #3
iant
The file runtime/cgo/gcc_arm.S is not compiled by cgo, so this CL is not going to ...
9 years, 5 months ago (2014-10-21 23:29:19 UTC) #4
minux
LGTM. Let's just put a hand assembled blx instruction as .word 0xe12fff35 in place of ...
9 years, 5 months ago (2014-10-21 23:29:33 UTC) #5
minux
On Oct 21, 2014 7:29 PM, <iant@golang.org> wrote: > The file runtime/cgo/gcc_arm.S is not compiled ...
9 years, 5 months ago (2014-10-21 23:31:19 UTC) #6
dfc
@iant, yes you are correct, this makes the previous commit doubly wrong, I'll commit this ...
9 years, 5 months ago (2014-10-21 23:33:23 UTC) #7
minux
On Oct 21, 2014 7:31 PM, "minux" <minux@golang.org> wrote: > On Oct 21, 2014 7:29 ...
9 years, 5 months ago (2014-10-21 23:33:23 UTC) #8
minux
On Oct 21, 2014 7:33 PM, "Dave Cheney" <dave@cheney.net> wrote: > @iant, yes you are ...
9 years, 5 months ago (2014-10-21 23:35:01 UTC) #9
dfc
> wait. I think after iant's suggested fix, the problem should be gone. I've committed ...
9 years, 5 months ago (2014-10-21 23:38:30 UTC) #10
dfc
*** Submitted as https://code.google.com/p/go/source/detail?r=3a5749e6ab94 *** undo CL 156430044 / 5d69cad4faaf Partial undo, changes to ldelf.c ...
9 years, 5 months ago (2014-10-21 23:42:39 UTC) #11
gobot
This changed caused perf changes on windows-amd64-perf: json-1 old new delta rss 69197824 75833344 +9.59 ...
9 years, 5 months ago (2014-10-22 00:54:15 UTC) #12
dfc
Bullshit. On Wed, Oct 22, 2014 at 11:54 AM, <gobot@golang.org> wrote: > This changed caused ...
9 years, 5 months ago (2014-10-22 00:58:12 UTC) #13
gobot
9 years, 5 months ago (2014-10-23 23:10:45 UTC) #14
Message was sent while issue was closed.
This CL appears to have broken the plan9-amd64-aram builder.
See http://build.golang.org/log/61be84bcd3dd4b8ecd40be55af76d3cbe8a85a98
Sign in to reply to this message.

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