code review 72080043: cmd/go: respect system CGO_CFLAGS and CGO_CXXFLAGS

cmd/go: respect system CGO_CFLAGS and CGO_CXXFLAGS

Fixes issue 6882

[atom, 2014-03-06 16:31:39]

Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com),

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

[iant, 2014-03-06 17:25:13]

Those flags should already be there when the compiler is invoked on a .c file
generated by cgo.  When does this change make a difference?

[atom, 2014-03-06 17:27:54]

On 2014/03/06 17:25:13, iant wrote:
> Those flags should already be there when the compiler is invoked on a .c file
> generated by cgo.  When does this change make a difference?

The only difference is that it is now possible to avoid the -g and -O2 options
to be passed to $CC.

[iant, 2014-03-06 17:58:47]

On Thu, Mar 6, 2014 at 9:27 AM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
> On 2014/03/06 17:25:13, iant wrote:
>>
>> Those flags should already be there when the compiler is invoked on a
>
> .c file
>>
>> generated by cgo.  When does this change make a difference?
>
>
> The only difference is that it is now possible to avoid the -g and -O2
> options to be passed to $CC.

Makes sense, but if I'm reading your patch correctly it will cause all
the CGO_CFLAGS options to be passed twice when using cgo.  If I'm
right, can we change that around somehow?

Ian

[atom, 2014-03-06 18:06:00]

On 2014/03/06 17:58:47, iant wrote:
> Makes sense, but if I'm reading your patch correctly it will cause all
> the CGO_CFLAGS options to be passed twice when using cgo.  If I'm
> right, can we change that around somehow?

The line

  if os.Getenv(envflags) == "" { ... }

only checks whether CGO_CFLAGS or CGO_CXXFLAGS environment variable exists, but
it does not read its value. The value is read and used elsewhere and this patch
does not touch that place.

Basically, the patch turns off -O2 -g when CGO_CFLAGS/CGO_CXXFLAGS is defined by
the environment.

I tested it with "go build -x a.go" and it works as expected.

[iant, 2014-03-06 18:41:04]

On Thu, Mar 6, 2014 at 10:06 AM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
>
> The line
>
>   if os.Getenv(envflags) == "" { ... }
>
> only checks whether CGO_CFLAGS or CGO_CXXFLAGS environment variable
> exists, but it does not read its value. The value is read and used
> elsewhere and this patch does not touch that place.
>
> Basically, the patch turns off -O2 -g when CGO_CFLAGS/CGO_CXXFLAGS is
> defined by the environment.

Sorry, I missed that.

In that case, what happens when using SWIG if CGO_CFLAGS is set in the
environment?

Ian

[atom, 2014-03-06 19:09:44]

Hello rsc@golang.org, iant@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[atom, 2014-03-07 11:28:30]

On 2014/03/06 18:41:04, iant wrote:
> In that case, what happens when using SWIG if CGO_CFLAGS is set in the
> environment?

I updated the patch to support SWIG. Please take another look.

[iant, 2014-03-07 18:48:56]

This seems like a minimal change but to me it doesn't seem like the right
change.  Having code that changes its behaviour based on whether an environment
variable exists seems odd.  I think it would make more sense to change cgo (and
swig) so that if CGO_CFLAGS is not defined in the environment, we use -g -O2
there.

I'm sorry for carping about this, but the use of CGO_CFLAGS is already hard to
understand and this seems to me to make it harder.

The main issue I see with that is whether it is important to pass -O2 when
linking.  When using link-time-optimization that may matter.  Traditionally
CFLAGS (and LDFLAGS) is passed when linking.  We don't seem to support that--the
place to support it, if we did, would be in -extldflags passed to the Go linker,
or possibly in the linker itself.  I guess we can treat that as a separate
issue.

https://codereview.appspot.com/72080043/diff/70001/src/cmd/go/build.go
File src/cmd/go/build.go (right):

https://codereview.appspot.com/72080043/diff/70001/src/cmd/go/build.go#newcod...
src/cmd/go/build.go:2466: if cgoCFLAGS := envList("CGO_CFLAGS"); len(cgoCFLAGS)
> 0 {
Should check cxx here.

[atom, 2014-03-07 19:47:06]

On 2014/03/07 18:48:56, iant wrote:
> This seems like a minimal change but to me it doesn't seem like the right
> change.  Having code that changes its behaviour based on whether an
environment
> variable exists seems odd.  I think it would make more sense to change cgo
(and
> swig) so that if CGO_CFLAGS is not defined in the environment, we use -g -O2
> there.
> 
> I'm sorry for carping about this, but the use of CGO_CFLAGS is already hard to
> understand and this seems to me to make it harder.
> 
> The main issue I see with that is whether it is important to pass -O2 when
> linking.  When using link-time-optimization that may matter.  Traditionally
> CFLAGS (and LDFLAGS) is passed when linking.  We don't seem to support
that--the
> place to support it, if we did, would be in -extldflags passed to the Go
linker,
> or possibly in the linker itself.  I guess we can treat that as a separate
> issue.

I agree that the code is a bit odd. I failed to pass CFLAGS to SWIG during
linking.

If there is agreement that we should scrap this CL, I will do so and will move
onto the next issue. Otherwise I will try to find a way how to pass CGO_CFLAGS
to the linker.

> https://codereview.appspot.com/72080043/diff/70001/src/cmd/go/build.go
> File src/cmd/go/build.go (right):
> 
>
https://codereview.appspot.com/72080043/diff/70001/src/cmd/go/build.go#newcod...
> src/cmd/go/build.go:2466: if cgoCFLAGS := envList("CGO_CFLAGS");
len(cgoCFLAGS)
> > 0 {
> Should check cxx here.

Done.

[iant, 2014-03-07 20:12:02]

On Fri, Mar 7, 2014 at 11:47 AM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
>
> I agree that the code is a bit odd. I failed to pass CFLAGS to SWIG
> during linking.
>
> If there is agreement that we should scrap this CL, I will do so and
> will move onto the next issue. Otherwise I will try to find a way how to
> pass CGO_CFLAGS to the linker.

I suppose that I would like to see the idea of this CL implemented
differently, by setting CGO_CFLAGS to a default value rather than by
testing later on whether it was set or not.  And set aside the issue
of passing CGO_CFLAGS to the linker to a later CL.

Ian

[atom, 2014-03-07 20:18:21]

On 2014/03/07 20:12:02, iant wrote:
> On Fri, Mar 7, 2014 at 11:47 AM,  <mailto:0xE2.0x9A.0x9B@gmail.com> wrote:
> >
> > I agree that the code is a bit odd. I failed to pass CFLAGS to SWIG
> > during linking.
> >
> > If there is agreement that we should scrap this CL, I will do so and
> > will move onto the next issue. Otherwise I will try to find a way how to
> > pass CGO_CFLAGS to the linker.
> 
> I suppose that I would like to see the idea of this CL implemented
> differently, by setting CGO_CFLAGS to a default value rather than by
> testing later on whether it was set or not.  And set aside the issue
> of passing CGO_CFLAGS to the linker to a later CL.
> 
> Ian

Ok. I will give this CL another try.

[atom, 2014-03-08 09:05:56]

I moved "-O2 -g" from the function to its callers. This duplicates some code,
but it makes its meaning easier to understand.

All (cgo, swig) should work as expected, including passing CFLAGS+LDFLAGS to
swig's final link.

I am using the following commands for testing:

CGO_CFLAGS="-O1" go build -x a.go 2>&1 | grep "gcc\|g++\|ld\|-O"

(cd $GOROOT/misc/swig/stdio; CGO_CFLAGS="-O1" CGO_LDFLAGS="-O1" go build -x 2>&1
| grep "gcc\|g++\|ld\|-O\|=")

(cd $GOROOT/misc/swig/callback; CGO_CXXFLAGS="-O1" CGO_LDFLAGS="-O1" go build -x
2>&1 | grep "gcc\|g++\|ld\|-O\|=")

$ cat a.go
package main
// #cgo LDFLAGS: -lm
// #include <math.h>
import "C"
func main() {
        println(C.sqrt(2))
}

Please take another look.

[iant, 2014-03-08 23:30:02]

On Sat, Mar 8, 2014 at 1:05 AM,  <0xE2.0x9A.0x9B@gmail.com> wrote:
>
> I moved "-O2 -g" from the function to its callers. This duplicates some
> code, but it makes its meaning easier to understand.>
> All (cgo, swig) should work as expected, including passing
> CFLAGS+LDFLAGS to swig's final link.

This version still does what I consider odd: first fetch the
environment variable, then check whether it is empty.  I sketched out
a change that is more what I had in mind:
https://codereview.appspot.com/72820045 .

Interestingly, this change fails on my system in misc/cgo/test.
Running go test -x in that directory ends like the appended.  The
difference is that with this change the go tool passes -g -O2 to cgo,
and that causes cgo to fail to understand the output.  Do you see this
failure with your change?

Ian

/home/iant/go/pkg/tool/linux_amd64/cgo -objdir
$WORK/_/home/iant/go/misc/cgo/test/_test/_obj_test/ -- -I
$WORK/_/home/iant/go/misc/cgo/test/_test/_obj_test/ -g -O2
-DCOMMON_VALUE=123 -DIS_WINDOWS=0 -Werror -pthread align.go api.go
basic.go callback.go cflags.go cthread.go duplicate_symbol.go env.go
exports.go fpvar.go helpers.go issue1222.go issue1328.go issue1560.go
issue1635.go issue2462.go issue3250.go issue3261.go issue3729.go
issue3741.go issue3775.go issue3945.go issue4029.go issue4054a.go
issue4054b.go issue4339.go issue4417.go issue4857.go issue5227.go
issue5337.go issue5548.go issue5603.go issue5740.go issue5986.go
issue6128.go issue6390.go issue6472.go issue6506.go issue6612.go
issue6833.go issue6997_linux.go setgid_linux.go
# _/home/iant/go/misc/cgo/test
./issue3729.go:52:8: error: enumerator value for '__cgo_enum__1' is
not an integer constant
./issue3729.go:55:8: error: enumerator value for '__cgo_enum__3' is
not an integer constant
./issue3729.go:57:8: error: enumerator value for '__cgo_enum__4' is
not an integer constant
./issue3729.go:61:8: error: enumerator value for '__cgo_enum__7' is
not an integer constant
./issue3729.go:64:8: error: enumerator value for '__cgo_enum__9' is
not an integer constant
./issue3729.go:66:8: error: enumerator value for '__cgo_enum__10' is
not an integer constant
./issue3729.go:70:2: error: initializer element is not constant
./issue3729.go:70:2: error: (near initialization for '__cgodebug_data[1]')
./issue3729.go:72:2: error: initializer element is not constant
./issue3729.go:72:2: error: (near initialization for '__cgodebug_data[3]')
./issue3729.go:73:2: error: initializer element is not constant
./issue3729.go:73:2: error: (near initialization for '__cgodebug_data[4]')
./issue3729.go:76:2: error: initializer element is not constant
./issue3729.go:76:2: error: (near initialization for '__cgodebug_data[7]')
./issue3729.go:78:2: error: initializer element is not constant
./issue3729.go:78:2: error: (near initialization for '__cgodebug_data[9]')
./issue3729.go:79:2: error: initializer element is not constant
./issue3729.go:79:2: error: (near initialization for '__cgodebug_data[10]')
$WORK/_/home/iant/go/misc/cgo/test/_test/test.test
FAIL	_/home/iant/go/misc/cgo/test [build failed]

[atom, 2014-03-09 07:34:32]

On 2014/03/08 23:30:02, iant wrote:
> On Sat, Mar 8, 2014 at 1:05 AM,  <mailto:0xE2.0x9A.0x9B@gmail.com> wrote:
> >
> > I moved "-O2 -g" from the function to its callers. This duplicates some
> > code, but it makes its meaning easier to understand.>
> > All (cgo, swig) should work as expected, including passing
> > CFLAGS+LDFLAGS to swig's final link.
> 
> This version still does what I consider odd: first fetch the
> environment variable, then check whether it is empty.  I sketched out
> a change that is more what I had in mind:
> https://codereview.appspot.com/72820045 .

Thanks. I think your CL is better than mine.

I can update this CL with your code, or alternatively you can submit your CL.

[atom, 2014-03-09 17:45:04]

On 2014/03/08 23:30:02, iant wrote:
> 
> https://codereview.appspot.com/72820045 .

I updated this CL with your code, and added code that passes cgoexeFLAGS to cgo.

all.bash terminates successfully (it takes about 9 minutes to run it on my
machine).

Please take another look.

[iant, 2014-03-12 01:56:22]

LGTM

Thanks.

[atom, 2014-03-12 06:16:31]

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

cmd/go: respect system CGO_CFLAGS and CGO_CXXFLAGS

Fixes issue 6882

LGTM=iant
R=rsc, iant
CC=golang-codereviews
https://codereview.appspot.com/72080043