code review 13247046: cgo: enable cgo on dragonfly

cgo: enable cgo on dragonfly

Enable cgo for dragonfly/386 and dragonfly/amd64.

[varialus, 2013-08-30 05:27:37]

Hello golang-dev@googlegroups.com,

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

[jsing, 2013-08-30 15:39:17]

https://codereview.appspot.com/13247046/diff/3001/src/pkg/go/build/build.go
File src/pkg/go/build/build.go (right):

https://codereview.appspot.com/13247046/diff/3001/src/pkg/go/build/build.go#n...
src/pkg/go/build/build.go:263: "dragonfly/amd64":  true,
You need to run gofmt on this.

https://codereview.appspot.com/13247046/diff/3001/src/run.bash
File src/run.bash (right):

https://codereview.appspot.com/13247046/diff/3001/src/run.bash#newcode107
src/run.bash:107: if [ "$GOHOSTOS-$GOARCH" != "dragonfly-amd64" ] || [
"$CGO_ENABLED" != 1 ]; then
We shouldn't need to check $CGO_ENABLED here - this was already done on line
104.

[varialus, 2013-08-30 16:53:50]

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

Please take another look.

[iant, 2013-08-30 18:21:54]

https://codereview.appspot.com/13247046/diff/15001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/13247046/diff/15001/src/cmd/ld/lib.c#newcode565
src/cmd/ld/lib.c:565: #if !defined(__DragonFly__)
This is incorrect.  It's a host/target confusion--that is to say, it will do the
wrong thing when building a cross-compiler.  You need to check HEADTYPE in
ldhostobj at runtime, rather than using a compile-time #ifdef.

https://codereview.appspot.com/13247046/diff/15001/src/pkg/runtime/cgo/gcc_dr...
File src/pkg/runtime/cgo/gcc_dragonfly_amd64.c (right):

https://codereview.appspot.com/13247046/diff/15001/src/pkg/runtime/cgo/gcc_dr...
src/pkg/runtime/cgo/gcc_dragonfly_amd64.c:1: // Copyright 2009 The Go Authors. 
All rights reserved.
Is there any change in this file?  If not, please use "hg change" to remove it
from the CL.

https://codereview.appspot.com/13247046/diff/15001/src/run.bash
File src/run.bash (right):

https://codereview.appspot.com/13247046/diff/15001/src/run.bash#newcode107
src/run.bash:107: if [ "$GOHOSTOS-$GOARCH" != "dragonfly-amd64" ]; then
Please use the same format as elsewhere in this file, which will be something
like
[ "$GOHOSTOS" == "dragonfly" ] ||
go test -ldflags '-linkmode=internal' || exit 1

[varialus, 2013-08-30 20:08:48]

https://codereview.appspot.com/13247046/diff/15001/src/pkg/runtime/cgo/gcc_dr...
File src/pkg/runtime/cgo/gcc_dragonfly_amd64.c (right):

https://codereview.appspot.com/13247046/diff/15001/src/pkg/runtime/cgo/gcc_dr...
src/pkg/runtime/cgo/gcc_dragonfly_amd64.c:1: // Copyright 2009 The Go Authors. 
All rights reserved.

This is a copied file. There are no changes to the contents of it. Running "hg
change" doesn't remove it.

On 2013/08/30 18:21:55, iant wrote:
> Is there any change in this file?  If not, please use "hg change" to remove it
> from the CL.

[varialus, 2013-08-30 20:09:07]

Hello golang-dev@googlegroups.com, jsing@google.com, iant@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[varialus, 2013-08-30 20:36:38]

Oops, when I fixed this I accidentally used getgoos() instead of checking the
HEADTYPE. Is the way I fixed it ok or should I fix it again to check the
HEADTYPE?

https://codereview.appspot.com/13247046/diff/15001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/13247046/diff/15001/src/cmd/ld/lib.c#newcode565
src/cmd/ld/lib.c:565: #if !defined(__DragonFly__)

Oops, when I fixed this I accidentally used getgoos() instead of checking the
HEADTYPE. Is the way I fixed it ok or should I fix it again to check the
HEADTYPE?

On 2013/08/30 18:21:55, iant wrote:
> This is incorrect.  It's a host/target confusion--that is to say, it will do
the
> wrong thing when building a cross-compiler.  You need to check HEADTYPE in
> ldhostobj at runtime, rather than using a compile-time #ifdef.

[varialus, 2013-08-30 20:54:39]

Hello golang-dev@googlegroups.com, jsing@google.com, iant@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[varialus, 2013-08-30 21:01:30]

https://codereview.appspot.com/13247046/diff/15001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/13247046/diff/15001/src/cmd/ld/lib.c#newcode565
src/cmd/ld/lib.c:565: #if !defined(__DragonFly__)
I updated it to check the HEADTYPE rather than using getgoos().

Sorry if this is considered spam since I already submitted the change. I'm not
used to this review process and all this "hg update", "hg mail", replying to
review comments and replying discussion threads feels like it's resulting in a
whole lot of emails getting sent out even though procedure wise it seems
correct.

On 2013/08/30 20:36:38, varialus wrote:
> 
> Oops, when I fixed this I accidentally used getgoos() instead of checking the
> HEADTYPE. Is the way I fixed it ok or should I fix it again to check the
> HEADTYPE?
> 
> On 2013/08/30 18:21:55, iant wrote:
> > This is incorrect.  It's a host/target confusion--that is to say, it will do
> the
> > wrong thing when building a cross-compiler.  You need to check HEADTYPE in
> > ldhostobj at runtime, rather than using a compile-time #ifdef.
>

[iant, 2013-08-30 21:42:28]

https://codereview.appspot.com/13247046/diff/35001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/13247046/diff/35001/src/cmd/ld/lib.c#newcode580
src/cmd/ld/lib.c:580: if( !((strcmp(pkg, "net") == 0) || (strcmp(pkg, "os/user")
== 0)) || HEADTYPE != Hdragonfly )
This is confusing and indirect (and it needs a comment).  I think you should
leave this loop alone.  Instead, after the code that sets externalobj = 1, I
think you should write

        /* On Dragonfly we need to do this because.... */
        if(HEADTYPE == Hdragonfly)
        if(strcmp(pkg, "net") == 0 || strcmp(pkg, "os/user") == 0)
                externalobj = 1;

https://codereview.appspot.com/13247046/diff/35001/src/run.bash
File src/run.bash (right):

https://codereview.appspot.com/13247046/diff/35001/src/run.bash#newcode107
src/run.bash:107: [ "$GOHOSTOS-$GOARCH" = "dragonfly-amd64" ] ||
As far as I can see you should only test GOHOSTOS here, not GOARCH.  When and if
the dragonfly port supports other processors, they will presumably encounter
exactly the same issue.

https://codereview.appspot.com/13247046/diff/35001/src/run.bash#newcode109
src/run.bash:109: ) || exit $?
I don't think you need the parentheses or the || exit $?.

[varialus, 2013-08-31 00:01:38]

https://codereview.appspot.com/13247046/diff/35001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/13247046/diff/35001/src/cmd/ld/lib.c#newcode580
src/cmd/ld/lib.c:580: if( !((strcmp(pkg, "net") == 0) || (strcmp(pkg, "os/user")
== 0)) || HEADTYPE != Hdragonfly )
I fixed this up as you recommended, but I set isinternal before it gets checked
rather than setting externalobj afer isinternal is checked because even if
isinternal isn't used anymore, I don't like leaving it set to the incorrect
value.

On 2013/08/30 21:42:28, iant wrote:
> This is confusing and indirect (and it needs a comment).  I think you should
> leave this loop alone.  Instead, after the code that sets externalobj = 1, I
> think you should write
> 
>         /* On Dragonfly we need to do this because.... */
>         if(HEADTYPE == Hdragonfly)
>         if(strcmp(pkg, "net") == 0 || strcmp(pkg, "os/user") == 0)
>                 externalobj = 1;

https://codereview.appspot.com/13247046/diff/35001/src/run.bash
File src/run.bash (right):

https://codereview.appspot.com/13247046/diff/35001/src/run.bash#newcode107
src/run.bash:107: [ "$GOHOSTOS-$GOARCH" = "dragonfly-amd64" ] ||
Yeah, presumably so. I'm fine with only checking GOHOSTOS, so that's what I've
done.

On 2013/08/30 21:42:28, iant wrote:
> As far as I can see you should only test GOHOSTOS here, not GOARCH.  When and
if
> the dragonfly port supports other processors, they will presumably encounter
> exactly the same issue.

https://codereview.appspot.com/13247046/diff/35001/src/run.bash#newcode109
src/run.bash:109: ) || exit $?
Without ethier of them it fails due to a syntax error. The parentheses seems to
match the preceding one. All the other instances had the || exit $? so I left it
in for consistency without testing it with the ) but without the || exit $?.

On 2013/08/30 21:42:28, iant wrote:
> I don't think you need the parentheses or the || exit $?.

[varialus, 2013-08-31 00:03:50]

Hello golang-dev@googlegroups.com, jsing@google.com, iant@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[iant, 2013-08-31 16:52:00]

https://codereview.appspot.com/13247046/diff/45001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/13247046/diff/45001/src/cmd/ld/lib.c#newcode586
src/cmd/ld/lib.c:586: // No cgo check because ldhostobj() not called while
CGO_ENABLED=0
Add periods at the end of the sentences.  Remove the () after ldhostobj.

I would actually like a longer comment here.  As I understand the core problem
is that the DragonFly libc uses "extern __thread int errno;" and 6l/8l don't
support __thread variables.  I'd like the comment to explain that.  The goal
should be to say what needs to be fixed in order to remove this condition.

https://codereview.appspot.com/13247046/diff/45001/src/cmd/ld/lib.c#newcode588
src/cmd/ld/lib.c:588: if( strcmp(pkg, "net") == 0 || strcmp(pkg, "os/user") == 0
)
Don't indent the second if line (and remove one tab from the assignment to is
internal).  In the C code used here pairs of if statements are used for && mixed
with ||.  For example see line 896 of ld/elf.c.

Also remove the spaces after the open parenthesis and before the close
parenthesis.

[varialus, 2013-08-31 20:11:02]

https://codereview.appspot.com/13247046/diff/45001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/13247046/diff/45001/src/cmd/ld/lib.c#newcode586
src/cmd/ld/lib.c:586: // No cgo check because ldhostobj() not called while
CGO_ENABLED=0
I don't know how to satisfactorily describe these errors, so I've asked for
help. https://groups.google.com/d/msg/golang-dev/O2ALUcNCTAQ/K_3mus822VkJ

https://codereview.appspot.com/13247046/diff/45001/src/cmd/ld/lib.c#newcode588
src/cmd/ld/lib.c:588: if( strcmp(pkg, "net") == 0 || strcmp(pkg, "os/user") == 0
)
No problem. My next upload will contain these changes as soon as I've resolved
the comment issue.

[varialus, 2013-09-01 00:04:04]

That last round was soul crushingly depressing.

Might have something to do how many fraking hours I spent porting Go and shortly
after getting it to build and the morning after I fixed six failing sets of
tests, jsing submitted the completed port with all tests passing, with a kindly
you're so welcome and silence when asked about attribution. Of course I did ask
for help and I did say that I'd rather be using Go than working on Go, so I am
nonetheless grateful.

Anyway, no, I don't have a fantastic understanding of the errors which I worked
around. If I did, I wouldn't have worked around them. I'm just a complete noob
with zero Go/C experience but I've worked my ass off the last few weeks and I
just want to be done with at least one ounce of attribution.
https://groups.google.com/d/msg/golang-dev/bwF5jTJmybM/2lCZo4DM9VIJ

Sorry to vent. It probably has nothing to do with you, nor jsing, nor even some
team leads' passive aggressiveness toward the community. I'm probably just moody
for whatever reason allows people to dismiss other people's moodiness.

[bradfitz, 2013-09-01 17:00:28]

FWIW, you got a shout-out in
https://code.google.com/p/go/source/detail?r=fb7526b2f6d838cfab4ebfd2fe107c3a...

Half the fun of golang-dev is being humbled.



On Sat, Aug 31, 2013 at 5:04 PM, <varialus@gmail.com> wrote:

> That last round was soul crushingly depressing.
>
> Might have something to do how many fraking hours I spent porting Go and
> shortly after getting it to build and the morning after I fixed six
> failing sets of tests, jsing submitted the completed port with all tests
> passing, with a kindly you're so welcome and silence when asked about
> attribution. Of course I did ask for help and I did say that I'd rather
> be using Go than working on Go, so I am nonetheless grateful.
>
> Anyway, no, I don't have a fantastic understanding of the errors which I
> worked around. If I did, I wouldn't have worked around them. I'm just a
> complete noob with zero Go/C experience but I've worked my ass off the
> last few weeks and I just want to be done with at least one ounce of
> attribution.
>
https://groups.google.com/d/**msg/golang-dev/bwF5jTJmybM/**2lCZo4DM9VIJ<https...
>
> Sorry to vent. It probably has nothing to do with you, nor jsing, nor
> even some team leads' passive aggressiveness toward the community. I'm
> probably just moody for whatever reason allows people to dismiss other
> people's moodiness.
>
>
>
https://codereview.appspot.**com/13247046/<https://codereview.appspot.com/132...
>
> --
>
> ---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...
> .
>

[varialus, 2013-09-01 22:53:21]

Thank you pointing that out as I probably wouldn't have run into it otherwise.

I don't like that annoyances affect me as much as they do, nor do I like coming
off as a whiny little drama queen as I think I sometimes do. But I'm currently
struggling with some hopefully temporary health issues which sometimes affect my
mood in very real ways, and I don't have many social outlets, so although I
could just stay silent, I don't want to be any more reclusive than I already am.


On 2013/09/01 17:00:28, bradfitz wrote:
> FWIW, you got a shout-out in
>
https://code.google.com/p/go/source/detail?r=fb7526b2f6d838cfab4ebfd2fe107c3a...
> 
> Half the fun of golang-dev is being humbled.

[iant, 2013-09-03 13:34:19]

On 2013/09/01 00:04:04, varialus wrote:
> That last round was soul crushingly depressing.
> 
> Might have something to do how many fraking hours I spent porting Go and
shortly
> after getting it to build and the morning after I fixed six failing sets of
> tests, jsing submitted the completed port with all tests passing, with a
kindly
> you're so welcome and silence when asked about attribution. Of course I did
ask
> for help and I did say that I'd rather be using Go than working on Go, so I am
> nonetheless grateful.
> 
> Anyway, no, I don't have a fantastic understanding of the errors which I
worked
> around. If I did, I wouldn't have worked around them. I'm just a complete noob
> with zero Go/C experience but I've worked my ass off the last few weeks and I
> just want to be done with at least one ounce of attribution.
> https://groups.google.com/d/msg/golang-dev/bwF5jTJmybM/2lCZo4DM9VIJ
> 
> Sorry to vent. It probably has nothing to do with you, nor jsing, nor even
some
> team leads' passive aggressiveness toward the community. I'm probably just
moody
> for whatever reason allows people to dismiss other people's moodiness.

As far as I know this CL is still relevant.  jsing's recent CLs added 386
support but don't enable cgo.  Am I missing something?

As far as the comment goes, it would be enough to add a link to a mailing list
discussion.  The important point is that somebody seeing that code and that
comment needs to have some way to understand what needs to be fixed.  It's not
helpful to say "it doesn't work."

[jsing, 2013-09-03 18:57:15]

On 3 September 2013 23:34, <iant@golang.org> wrote:

> On 2013/09/01 00:04:04, varialus wrote:
>
>> That last round was soul crushingly depressing.
>>
>
>  Might have something to do how many fraking hours I spent porting Go
>>
> and shortly
>
>> after getting it to build and the morning after I fixed six failing
>>
> sets of
>
>> tests, jsing submitted the completed port with all tests passing, with
>>
> a kindly
>
>> you're so welcome and silence when asked about attribution. Of course
>>
> I did ask
>
>> for help and I did say that I'd rather be using Go than working on Go,
>>
> so I am
>
>> nonetheless grateful.
>>
>
>  Anyway, no, I don't have a fantastic understanding of the errors which
>>
> I worked
>
>> around. If I did, I wouldn't have worked around them. I'm just a
>>
> complete noob
>
>> with zero Go/C experience but I've worked my ass off the last few
>>
> weeks and I
>
>> just want to be done with at least one ounce of attribution.
>>
https://groups.google.com/d/**msg/golang-dev/bwF5jTJmybM/**2lCZo4DM9VIJ<https...
>>
>
>  Sorry to vent. It probably has nothing to do with you, nor jsing, nor
>>
> even some
>
>> team leads' passive aggressiveness toward the community. I'm probably
>>
> just moody
>
>> for whatever reason allows people to dismiss other people's moodiness.
>>
>
> As far as I know this CL is still relevant.  jsing's recent CLs added
> 386 support but don't enable cgo.  Am I missing something?
>

This CL is still relevant, although it would currently break dragonfly/386
since it only includes cgo support for dragonfly/amd64. Also, the change
to cmd/api/goapi.go probably should not be included as part of this CL.

As far as the comment goes, it would be enough to add a link to a
> mailing list discussion.  The important point is that somebody seeing
> that code and that comment needs to have some way to understand what
> needs to be fixed.  It's not helpful to say "it doesn't work."
>

I've updated my original change to include the cmd/ld/lib.c "fix" and
enable cgo on dragonfly/386 - this also includes a suitable comment re the
need to force external linkmode for the net and os/user packages:

  https://codereview.appspot.com/13213043/

If Varialus wants to pull these changes into this CL then we can submit it,
otherwise I can mail my CL for review.

[varialus, 2013-09-03 23:51:23]

On 2013/09/03 18:57:15, jsing wrote:
> On 3 September 2013 23:34, <mailto:iant@golang.org> wrote:
> 
> > On 2013/09/01 00:04:04, varialus wrote:
> >
> >> That last round was soul crushingly depressing.
> >>
> >
> >  Might have something to do how many fraking hours I spent porting Go
> >>
> > and shortly
> >
> >> after getting it to build and the morning after I fixed six failing
> >>
> > sets of
> >
> >> tests, jsing submitted the completed port with all tests passing, with
> >>
> > a kindly
> >
> >> you're so welcome and silence when asked about attribution. Of course
> >>
> > I did ask
> >
> >> for help and I did say that I'd rather be using Go than working on Go,
> >>
> > so I am
> >
> >> nonetheless grateful.
> >>
> >
> >  Anyway, no, I don't have a fantastic understanding of the errors which
> >>
> > I worked
> >
> >> around. If I did, I wouldn't have worked around them. I'm just a
> >>
> > complete noob
> >
> >> with zero Go/C experience but I've worked my ass off the last few
> >>
> > weeks and I
> >
> >> just want to be done with at least one ounce of attribution.
> >>
>
https://groups.google.com/d/**msg/golang-dev/bwF5jTJmybM/**2lCZo4DM9VIJ%3Chtt...>
> >>
> >
> >  Sorry to vent. It probably has nothing to do with you, nor jsing, nor
> >>
> > even some
> >
> >> team leads' passive aggressiveness toward the community. I'm probably
> >>
> > just moody
> >
> >> for whatever reason allows people to dismiss other people's moodiness.
> >>
> >
> > As far as I know this CL is still relevant.  jsing's recent CLs added
> > 386 support but don't enable cgo.  Am I missing something?
> >
> 
> This CL is still relevant, although it would currently break dragonfly/386
> since it only includes cgo support for dragonfly/amd64. Also, the change
> to cmd/api/goapi.go probably should not be included as part of this CL.
> 
> As far as the comment goes, it would be enough to add a link to a
> > mailing list discussion.  The important point is that somebody seeing
> > that code and that comment needs to have some way to understand what
> > needs to be fixed.  It's not helpful to say "it doesn't work."
> >
> 
> I've updated my original change to include the cmd/ld/lib.c "fix" and
> enable cgo on dragonfly/386 - this also includes a suitable comment re the
> need to force external linkmode for the net and os/user packages:
> 
>   https://codereview.appspot.com/13213043/
> 
> If Varialus wants to pull these changes into this CL then we can submit it,
> otherwise I can mail my CL for review.

[varialus, 2013-09-04 00:02:34]

Sorry about that last message. I was on my phone and must have accidentally
touched the send button.

I would like to make these changes and resubmit the CL. It takes some effort for
me to switch between unfinished projects. I finally got switched to a new
project yesterday after a few days of lost productivity as I waited for a reply
about about the explanatory comments. I'll try to switch back to this project
and have it resubmitted before the end of tomorrow.

[varialus, 2013-09-04 21:50:42]

Hello golang-dev@googlegroups.com, jsing@google.com, iant@golang.org,
bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[iant, 2013-09-04 22:17:47]

LGTM

Thanks for your patience on this.

[iant, 2013-09-04 22:19:25]

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

cgo: enable cgo on dragonfly

Enable cgo for dragonfly/386 and dragonfly/amd64.

R=golang-dev, jsing, iant, bradfitz
CC=golang-dev
https://codereview.appspot.com/13247046

Committer: Ian Lance Taylor <iant@golang.org>