code review 3533041: Fix windows build.

Fix windows build.

[vcc, 2010-12-09 05:13:35]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[brainman, 2010-12-09 12:36:15]

LGTM. You're too quick <g>!

[brainman, 2010-12-09 12:36:25]

http://codereview.appspot.com/3533041/diff/2001/src/cmd/8l/obj.c
File src/cmd/8l/obj.c (right):

http://codereview.appspot.com/3533041/diff/2001/src/cmd/8l/obj.c#newcode589
src/cmd/8l/obj.c:589: if(s->type != SBSS && !s->dupok && !(s->type == SDYNIMPORT
&& HEADTYPE == 10)) {
I don't think you should change this line. Instead just change all code like:

void *runtime·VirtualAlloc;

into

extern void *runtime·VirtualAlloc;

in runtime/windows/mem.c and runtime/windows/thread.c. This way, I think, it is
more in line with how pragma "dynimport" is used everywhere else.

http://codereview.appspot.com/3533041/diff/2001/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/3533041/diff/2001/src/pkg/Makefile#newcode142
src/pkg/Makefile:142: ifneq ($(GOOS),windows)
Should put some comment here to remind us to fix it later, or, perhaps, create
an issue.

[vcc, 2010-12-09 13:43:07]

Fixed runtime/cgo build, please take another look.

To make new cgo work on windows, we also need link PE object file in 8l. I just
look into ldelf.c and ldmacho.c, I will try to do it if I get time. 

I think make cgo create dlls also ok on windows, we use many dlls on windows.
Maybe provide a option to do old cgo way is useful?

http://codereview.appspot.com/3533041/diff/2001/src/cmd/8l/obj.c
File src/cmd/8l/obj.c (right):

http://codereview.appspot.com/3533041/diff/2001/src/cmd/8l/obj.c#newcode589
src/cmd/8l/obj.c:589: if(s->type != SBSS && !s->dupok && !(s->type == SDYNIMPORT
&& HEADTYPE == 10)) {
On 2010/12/09 12:36:25, brainman wrote:
> I don't think you should change this line. Instead just change all code like:
> 
> void *runtime·VirtualAlloc;
> 
> into
> 
> extern void *runtime·VirtualAlloc;
> 
> in runtime/windows/mem.c and runtime/windows/thread.c. This way, I think, it
is
> more in line with how pragma "dynimport" is used everywhere else.

Done.

http://codereview.appspot.com/3533041/diff/2001/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/3533041/diff/2001/src/pkg/Makefile#newcode142
src/pkg/Makefile:142: ifneq ($(GOOS),windows)
On 2010/12/09 12:36:25, brainman wrote:
> Should put some comment here to remind us to fix it later, or, perhaps, create
> an issue.

I fixed it, don't need make this change now.

[rsc, 2010-12-09 15:17:06]

Wow, thanks for the quick fix!

> To make new cgo work on windows, we also need link PE object file in 8l.
> I just look into ldelf.c and ldmacho.c, I will try to do it if I get
> time.
>
> I think make cgo create dlls also ok on windows, we use many dlls on
> windows. Maybe provide a option to do old cgo way is useful?

I'd prefer not to do it that way.
The fact that other languages make it hard
to distribute binaries on Windows doesn't
mean that Go should.  All the same reasons
for avoiding it on Linux and OS X still hold
here (we use many shared libraries on Linux too).

Russ

[rsc, 2010-12-09 18:20:50]

> http://codereview.appspot.com/3533041/diff/2001/src/cmd/8l/obj.c#newcode589
> src/cmd/8l/obj.c:589: if(s->type != SBSS && !s->dupok && !(s->type ==
> SDYNIMPORT && HEADTYPE == 10)) {
> I don't think you should change this line. Instead just change all code
> like:
>
> void *runtime·VirtualAlloc;
>
> into
>
> extern void *runtime·VirtualAlloc;
>
> in runtime/windows/mem.c and runtime/windows/thread.c. This way, I
> think, it is more in line with how pragma "dynimport" is used everywhere
> else.

Agreed.

> http://codereview.appspot.com/3533041/diff/2001/src/pkg/Makefile#newcode142
> src/pkg/Makefile:142: ifneq ($(GOOS),windows)
> Should put some comment here to remind us to fix it later, or, perhaps,
> create an issue.

I think there are enough reminders that cgo doesn't work
on Windows.  But do please change this block to

ifeq ($(GOOS),windows)
DIRS:=$(filter-out $(DIRS),runtime/cgo)
endif

instead of removing runtime/cgo above,
so that people not concerned with windows
don't have to remember that these few lines are here.

Thanks.

[vcc, 2010-12-13 02:47:00]

update, please take another look.

[rsc1, 2010-12-13 03:19:52]

LGTM

http://codereview.appspot.com/3533041/diff/18001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/3533041/diff/18001/src/cmd/ld/data.c#newcode169
src/cmd/ld/data.c:169: if (thechar != '8' || HEADTYPE != 10) // Windows PE
supports relocation references to dynamic import symbols
When does this get used?

[brainman, 2010-12-13 04:11:26]

http://codereview.appspot.com/3533041/diff/18001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/3533041/diff/18001/src/cmd/ld/data.c#newcode169
src/cmd/ld/data.c:169: if (thechar != '8' || HEADTYPE != 10) // Windows PE
supports relocation references to dynamic import symbols
On 2010/12/13 03:19:52, rsc1 wrote:
> When does this get used?

Without this "if", 8l prints many diag("unhandled relocation for ...") and
fails.

I don't know what is the right thing to do here, but to disable this diag, as
Wei did.

Perhaps, we should go ahead with this CL as is, and we could decide what to do
about it later.

[brainman, 2010-12-13 05:41:11]

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

Fix windows build.

R=brainman, rsc
CC=golang-dev
http://codereview.appspot.com/3533041

Committer: Alex Brainman <alex.brainman@gmail.com>