code review 7057063: cmd/go: get -m options from GOARCH when using gccgo

cmd/go: get -m options from GOARCH when using gccgo

[iant, 2013-01-09 22:29:19]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-01-09 22:38:47]

LGTM

On Wed, Jan 9, 2013 at 2:29 PM, <iant@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/go: get -m options from GOARCH when using gccgo
>
> Please review this at
https://codereview.appspot.**com/7057063/<https://codereview.appspot.com/7057...
>
> Affected files:
>   M src/cmd/go/build.go
>
>
> Index: src/cmd/go/build.go
> ==============================**==============================**=======
> --- a/src/cmd/go/build.go
> +++ b/src/cmd/go/build.go
> @@ -1404,6 +1404,7 @@
>         out := p.Name + ".o"
>         ofile = obj + out
>         gcargs := []string{"-g"}
> +       gcargs = append(gcargs, b.gccArchArgs()...)
>         if pkgpath := gccgoPkgpath(p); pkgpath != "" {
>                 gcargs = append(gcargs, "-fgo-pkgpath="+pkgpath)
>         }
> @@ -1423,6 +1424,7 @@
>         if pkgpath := gccgoCleanPkgpath(p); pkgpath != "" {
>                 defs = append(defs, `-D`, `GOPKGPATH="`+pkgpath+`"`)
>         }
> +       defs = append(defs, b.gccArchArgs()...)
>         return b.run(p.Dir, p.ImportPath, "gccgo", "-I", obj, "-o", ofile,
> defs, sfile)
>  }
>
> @@ -1446,7 +1448,7 @@
>         // and all LDFLAGS from cgo dependencies.
>         afiles := make(map[*Package]string)
>         sfiles := make(map[*Package][]string)
> -       ldflags := []string{}
> +       ldflags := b.gccArchArgs()
>         cgoldflags := []string{}
>         usesCgo := false
>         for _, a := range allactions {
> @@ -1487,6 +1489,7 @@
>         inc := filepath.Join(goroot, "pkg", fmt.Sprintf("%s_%s", goos,
> goarch))
>         cfile = mkAbs(p.Dir, cfile)
>         defs := []string{"-D", "GOOS_" + goos, "-D", "GOARCH_" + goarch}
> +       defs = append(defs, b.gccArchArgs()...)
>         if pkgpath := gccgoCleanPkgpath(p); pkgpath != "" {
>                 defs = append(defs, `-D`, `GOPKGPATH="`+pkgpath+`"`)
>         }
> @@ -1547,14 +1550,7 @@
>         if goos != "windows" {
>                 a = append(a, "-fPIC")
>         }
> -       switch archChar {
> -       case "8":
> -               a = append(a, "-m32")
> -       case "6":
> -               a = append(a, "-m64")
> -       case "5":
> -               a = append(a, "-marm") // not thumb
> -       }
> +       a = append(a, b.gccArchArgs()...)
>         // gcc-4.5 and beyond require explicit "-pthread" flag
>         // for multithreading with pthread library.
>         if buildContext.CgoEnabled {
> @@ -1576,6 +1572,19 @@
>         return a
>  }
>
> +// gccArchArgs returns arguments to pass to gcc based on the architecture.
> +func (b *builder) gccArchArgs() []string {
> +       switch archChar {
> +       case "8":
> +               return []string{"-m32"}
> +       case "6":
> +               return []string{"-m64"}
> +       case "5":
> +               return []string{"-marm"} // not thumb
> +       }
> +       return nil
> +}
> +
>  func envList(key string) []string {
>         return strings.Fields(os.Getenv(key))
>  }
>
>
>

[minux1, 2013-01-09 22:42:30]

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

https://codereview.appspot.com/7057063/diff/1/src/cmd/go/build.go#newcode1583
src/cmd/go/build.go:1583: return []string{"-marm"} // not thumb
does gccgo require -marm for ARM?
-marm is only necessary because 5l can't handle
thumb library yet and i think this doesn't apply
to gccgo.

[iant, 2013-01-09 22:45:10]

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

cmd/go: get -m options from GOARCH when using gccgo

R=golang-dev, bradfitz, minux.ma
CC=golang-dev
https://codereview.appspot.com/7057063

[iant, 2013-01-09 23:21:16]

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

https://codereview.appspot.com/7057063/diff/1/src/cmd/go/build.go#newcode1583
src/cmd/go/build.go:1583: return []string{"-marm"} // not thumb
On 2013/01/09 22:42:30, minux wrote:
> does gccgo require -marm for ARM?
> -marm is only necessary because 5l can't handle
> thumb library yet and i think this doesn't apply
> to gccgo.

That's a fair point but I think it does no harm and it keeps the code together. 
In order to get thumb code you need to pass an explicit -mthumb, and if that
worked before (using --gccgoflags) it should continue to work.  I don't know
whether it works, but this shouldn't affect it.