Issue 28050043: code review 28050043: cmd/go: When linking with gccgo pass .a files in the or... (Closed)
Descriptioncmd/go: When linking with gccgo pass .a files in the order they are discovered
Under some circumstances linking a test binary with gccgo can fail, because
the installed version of the library ends up before the version built for the
test on the linker command line.
This admittedly slightly hackish fix fixes this by putting the library archives
on the linker command line in the order that a pre-order depth first traversal
of the dependencies gives them, which has the side effect of always putting the
version of the library built for the test first.
Fixes issue 6768
Patch Set 1 #Patch Set 2 : diff -r b1edf8faa5d6 https://code.google.com/p/go/ #Patch Set 3 : diff -r c8d3de543c1b https://code.google.com/p/go/ #
Total comments: 2
Patch Set 4 : diff -r 7abe32ccffb1 https://code.google.com/p/go/ #MessagesTotal messages: 17
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
can we solve this problem by surrounding the archive files with -Wl,--start-group and -Wl,--end-group?
Sign in to reply to this message.
On 2013/11/18 00:42:58, minux wrote: > can we solve this problem by surrounding the archive files with > -Wl,--start-group and -Wl,--end-group?
Sign in to reply to this message.
On 2013/11/18 00:42:58, minux wrote: > can we solve this problem by surrounding the archive files with > -Wl,--start-group and -Wl,--end-group? No, we already do that. I think perhaps with this change we could stop though! The problem comes when the installed version of the library does not define some symbols used in the test and then you get duplicate symbols. Did you read the issue? The explanation I made there may make more sense.
Sign in to reply to this message.
Ping?
Sign in to reply to this message.
Replacing golang-dev with golang-codereviews.
Sign in to reply to this message.
R=rsc@golang.org (assigned by dave@cheney.net)
Sign in to reply to this message.
LGTM https://codereview.appspot.com/28050043/diff/40001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/28050043/diff/40001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1701: afiles_seen := make(map[*Package]bool) afilesSeen please do the same for sfiles, so that that order is consistent too.
Sign in to reply to this message.
Michael. If you want to address rsc's comments, I'll submit this for you when ready. On Wed, Jan 22, 2014 at 3:03 PM, <rsc@golang.org> wrote: > LGTM > > > > https://codereview.appspot.com/28050043/diff/40001/src/cmd/go/build.go > File src/cmd/go/build.go (right): > > https://codereview.appspot.com/28050043/diff/40001/src/ > cmd/go/build.go#newcode1701 > src/cmd/go/build.go:1701: afiles_seen := make(map[*Package]bool) > afilesSeen > > please do the same for sfiles, so that that order is consistent too. > > > https://codereview.appspot.com/28050043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. >
Sign in to reply to this message.
https://codereview.appspot.com/28050043/diff/40001/src/cmd/go/build.go File src/cmd/go/build.go (right): https://codereview.appspot.com/28050043/diff/40001/src/cmd/go/build.go#newcod... src/cmd/go/build.go:1701: afiles_seen := make(map[*Package]bool) On 2014/01/22 04:03:05, rsc wrote: > afilesSeen > > please do the same for sfiles, so that that order is consistent too. > Done, I think. sfiles is a bit different it seems? But I've made the code simpler and more consistent -- PTAL.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=369547fa0881 *** cmd/go: When linking with gccgo pass .a files in the order they are discovered Under some circumstances linking a test binary with gccgo can fail, because the installed version of the library ends up before the version built for the test on the linker command line. This admittedly slightly hackish fix fixes this by putting the library archives on the linker command line in the order that a pre-order depth first traversal of the dependencies gives them, which has the side effect of always putting the version of the library built for the test first. Fixes issue 6768 LGTM=rsc R=golang-codereviews, minux.ma, gobot, rsc, dave CC=golang-codereviews https://codereview.appspot.com/28050043 Committer: Dave Cheney <dave@cheney.net>
Sign in to reply to this message.
This CL appears to have broken the linux-amd64-race builder.
Sign in to reply to this message.
Interesting. I can't make the linux race build fail on my machine. On Tue, Jan 28, 2014 at 4:52 PM, <gobot@golang.org> wrote: > This CL appears to have broken the linux-amd64-race builder. > > > https://codereview.appspot.com/28050043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. >
Sign in to reply to this message.
On Tue, Jan 28, 2014 at 9:52 AM, <gobot@golang.org> wrote: > This CL appears to have broken the linux-amd64-race builder. > > > https://codereview.appspot.com/28050043/ Andrew, can we have 'tail -n 200' and the link to full log here as before?
Sign in to reply to this message.
On Tue, Jan 28, 2014 at 2:18 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Tue, Jan 28, 2014 at 9:52 AM, <gobot@golang.org> wrote: > > This CL appears to have broken the linux-amd64-race builder. > Andrew, can we have 'tail -n 200' and the link to full log here as before? > I agree. I also suggested adding the link in the CL for this feature, but it's deferred. I think we really need the link here.
Sign in to reply to this message.
Is it this one ? http://build.golang.org/log/75fba5c70ac0a60131d5327018acb152a0897b92 > On 28 Jan 2014, at 18:18, Dmitry Vyukov <dvyukov@google.com> wrote: > >> On Tue, Jan 28, 2014 at 9:52 AM, <gobot@golang.org> wrote: >> This CL appears to have broken the linux-amd64-race builder. >> >> >> https://codereview.appspot.com/28050043/ > > > Andrew, can we have 'tail -n 200' and the link to full log here as before?
Sign in to reply to this message.
Filed https://code.google.com/p/go/issues/detail?id=7224 On Tue, Jan 28, 2014 at 9:52 AM, <gobot@golang.org> wrote: > This CL appears to have broken the linux-amd64-race builder. > > > https://codereview.appspot.com/28050043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
|
