code review 8716044: cmd/ld: always do external link for -linkmode=external

cmd/ld: always do external link for -linkmode=external

There are tests in run.bash for -linkmode=external.

Fixes issue 5238.

[iant, 2013-04-12 18:29:42]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2013-04-12 18:40:20]

I don't understand this part of the commit descriptionn: "There are tests
in run.bash for -linkmode=external."

If there are already tests, why is the dashboard not failing?  Are the
tests not run?


On Fri, Apr 12, 2013 at 11:29 AM, <iant@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cmd/ld: always do external link for -linkmode=external
>
> There are tests in run.bash for -linkmode=external.
>
> Fixes issue 5238.
>
> Please review this at
https://codereview.appspot.**com/8716044/<https://codereview.appspot.com/8716...
>
> Affected files:
>   M src/cmd/ld/lib.c
>
>
> Index: src/cmd/ld/lib.c
> ==============================**==============================**=======
> --- a/src/cmd/ld/lib.c
> +++ b/src/cmd/ld/lib.c
> @@ -45,6 +45,10 @@
>  static int     maxlibdir = 0;
>  static int     cout = -1;
>
> +// Set if we see an object compiled by the host compiler that is not
> +// from a package that is known to support internal linking mode.
> +static int     externalobj = 0;
> +
>  static void    hostlinksetup(void);
>
>  char*  goroot;
> @@ -295,6 +299,19 @@
>                 loadinternal("math");
>         if(flag_race)
>                 loadinternal("runtime/race");
> +       if(linkmode == LinkExternal) {
> +               // This indicates a user requested -linkmode=external.
> +               // The startup code uses an import of runtime/cgo to decide
> +               // whether to initialize the TLS.  So give it one.  This
> could
> +               // be handled differently but it's an unusual case.
> +               loadinternal("runtime/cgo");
> +               // Pretend that we really imported the package.
> +               // This will do no harm if we did in fact import it.
> +               s = lookup("go.importpath.runtime/**cgo.", 0);
> +               s->type = SDATA;
> +               s->dupok = 1;
> +               s->reachable = 1;
> +       }
>
>         for(i=0; i<libraryp; i++) {
>                 if(debug['v'])
> @@ -303,14 +320,11 @@
>                 objfile(library[i].file, library[i].pkg);
>         }
>
> -       if(linkmode == LinkExternal && !iscgo)
> -               linkmode = LinkInternal;
> -
> -       // If we got this far in automatic mode, there were no
> -       // cgo uses that suggest we need external mode.
> -       // Switch to internal.
>         if(linkmode == LinkAuto) {
> -               linkmode = LinkInternal;
> +               if(iscgo && externalobj)
> +                       linkmode = LinkExternal;
> +               else
> +                       linkmode = LinkInternal;
>         }
>
>         if(linkmode == LinkInternal) {
> @@ -532,8 +546,8 @@
>                 }
>         }
>
> -       if(!isinternal && linkmode == LinkAuto)
> -               linkmode = LinkExternal;
> +       if(!isinternal)
> +               externalobj = 1;
>
>         if(nhostobj >= mhostobj) {
>                 if(mhostobj == 0)
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[remyoudompheng, 2013-04-12 18:42:28]

The test for linkmode=external tests that it is not broken, but not
that it is indeed externally linked.

2013/4/12 Brad Fitzpatrick <bradfitz@golang.org>:
> I don't understand this part of the commit descriptionn: "There are tests in
> run.bash for -linkmode=external."
>
> If there are already tests, why is the dashboard not failing?  Are the tests
> not run?
>
>
> On Fri, Apr 12, 2013 at 11:29 AM, <iant@golang.org> wrote:
>>
>> Reviewers: golang-dev1,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> cmd/ld: always do external link for -linkmode=external
>>
>> There are tests in run.bash for -linkmode=external.
>>
>> Fixes issue 5238.
>>
>> Please review this at https://codereview.appspot.com/8716044/
>>
>> Affected files:
>>   M src/cmd/ld/lib.c
>>
>>
>> Index: src/cmd/ld/lib.c
>> ===================================================================
>> --- a/src/cmd/ld/lib.c
>> +++ b/src/cmd/ld/lib.c
>> @@ -45,6 +45,10 @@
>>  static int     maxlibdir = 0;
>>  static int     cout = -1;
>>
>> +// Set if we see an object compiled by the host compiler that is not
>> +// from a package that is known to support internal linking mode.
>> +static int     externalobj = 0;
>> +
>>  static void    hostlinksetup(void);
>>
>>  char*  goroot;
>> @@ -295,6 +299,19 @@
>>                 loadinternal("math");
>>         if(flag_race)
>>                 loadinternal("runtime/race");
>> +       if(linkmode == LinkExternal) {
>> +               // This indicates a user requested -linkmode=external.
>> +               // The startup code uses an import of runtime/cgo to
>> decide
>> +               // whether to initialize the TLS.  So give it one.  This
>> could
>> +               // be handled differently but it's an unusual case.
>> +               loadinternal("runtime/cgo");
>> +               // Pretend that we really imported the package.
>> +               // This will do no harm if we did in fact import it.
>> +               s = lookup("go.importpath.runtime/cgo.", 0);
>> +               s->type = SDATA;
>> +               s->dupok = 1;
>> +               s->reachable = 1;
>> +       }
>>
>>         for(i=0; i<libraryp; i++) {
>>                 if(debug['v'])
>> @@ -303,14 +320,11 @@
>>                 objfile(library[i].file, library[i].pkg);
>>         }
>>
>> -       if(linkmode == LinkExternal && !iscgo)
>> -               linkmode = LinkInternal;
>> -
>> -       // If we got this far in automatic mode, there were no
>> -       // cgo uses that suggest we need external mode.
>> -       // Switch to internal.
>>         if(linkmode == LinkAuto) {
>> -               linkmode = LinkInternal;
>> +               if(iscgo && externalobj)
>> +                       linkmode = LinkExternal;
>> +               else
>> +                       linkmode = LinkInternal;
>>         }
>>
>>         if(linkmode == LinkInternal) {
>> @@ -532,8 +546,8 @@
>>                 }
>>         }
>>
>> -       if(!isinternal && linkmode == LinkAuto)
>> -               linkmode = LinkExternal;
>> +       if(!isinternal)
>> +               externalobj = 1;
>>
>>         if(nhostobj >= mhostobj) {
>>                 if(mhostobj == 0)
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>>
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[iant, 2013-04-12 18:51:12]

On Fri, Apr 12, 2013 at 11:40 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I don't understand this part of the commit descriptionn: "There are tests in
> run.bash for -linkmode=external."
>
> If there are already tests, why is the dashboard not failing?  Are the tests
> not run?

The tests are run.  This is a slight change to the way that
-linkmode=external works: it is now always run even if runtime/cgo is
not imported.  I meant to imply that this CL is OK because the tests
continue to pass.

The effect of this change is to ensure that the external linker is
used; I'm not sure how to test that specifically.

[r, 2013-04-12 18:51:22]

LGTM i think

[iant, 2013-04-12 20:21:28]

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

cmd/ld: always do external link for -linkmode=external

There are tests in run.bash for -linkmode=external.

Fixes issue 5238.

R=golang-dev, bradfitz, remyoudompheng, r
CC=golang-dev
https://codereview.appspot.com/8716044