code review 5822050: cmd/go: add -fno-common by default on Darwin

cmd/go: add -fno-common by default on Darwin
        Fixes part of issue 3253.
        We still need to support scattered relocations though.

[minux1, 2012-03-14 20:48:01]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[bsiegert, 2012-03-14 21:08:34]

http://codereview.appspot.com/5822050/diff/1002/src/cmd/go/build.go
File src/cmd/go/build.go (right):

http://codereview.appspot.com/5822050/diff/1002/src/cmd/go/build.go#newcode1338
src/cmd/go/build.go:1338: // our macho linker doesn't support common symbols, so
disable it
Mach-O

[minux1, 2012-03-15 04:09:50]

PTAL.

[bsiegert, 2012-03-15 10:18:14]

LGTM, leaving for others.

[rsc, 2012-03-15 18:20:28]

I'm skeptical of this, although I do know that we've had problems
with Mach-O common symbols before in our C code.
(On OS X, you can't say int x; in two different C files linked
into the same binary.  One has to be extern.)

Ian, does this CL seem okay to you?

Thanks.
Russ

[rsc, 2012-03-15 19:26:55]

Aha, I dug through the repository and found this in our old Makefiles.

 # The -fno-common here is not necessary, but some compilers
 # on OS X seem to set it by default.  Setting it here keeps the build
 # working in that non-standard context.
HOST_CFLAGS=-fno-common -I"$(GOROOT)/include" $(HOST_EXTRA_CFLAGS)

Please make the code say

// On OS X, some of the compilers behave as if -fno-common
// is always set, and the Mach-O linker in 6l/8l assumes this.
// See http://golang.org/issue/3253.
if goos == darwin {
    a = append(a, "-fno-common")
}

Then it looks good to me.

[iant2, 2012-03-15 20:33:04]

FYI

Russ Cox <rsc@golang.org> writes:

> I'm skeptical of this, although I do know that we've had problems
> with Mach-O common symbols before in our C code.
> (On OS X, you can't say int x; in two different C files linked
> into the same binary.  One has to be extern.)
>
> Ian, does this CL seem okay to you?

Given what you just said about OS X, yes.

Ian

[rsc, 2012-03-16 16:00:57]

LGTM

I will submit with the comment change I suggested.

[rsc, 2012-03-16 16:05:14]

*** Submitted as http://code.google.com/p/go/source/detail?r=d88577489c3d ***

cmd/go: add -fno-common by default on Darwin
        Fixes part of issue 3253.
        We still need to support scattered relocations though.

R=golang-dev, bsiegert, rsc, iant
CC=golang-dev
http://codereview.appspot.com/5822050

Committer: Russ Cox <rsc@golang.org>