code review 3733046: new cgo windows port.

new cgo windows port.

[vcc, 2010-12-25 06:13:25]

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

I'd like you to review this change.

[peterGo, 2010-12-26 00:28:17]

For this package,

gosqlite: trivial sqlite3 binding for Go 
http://code.google.com/p/gosqlite/

compiling this program,

package main

import (
	"fmt"
	sqlite "gosqlite"
)

func main() {
	db, err := sqlite.Open("gosqlite.db")
	fmt.Println(db, err)
}

fails when linking,

$ 8l gosqlite.8

adddynlib: unsupported binary format

adddynlib: unsupported binary format

p:/cgo/pkg/windows_386/gosqlite.a(sqlite.cgo2.o): elf object but not 386

p:/cgo/pkg/windows_386/gosqlite.a(_cgo_export.o): elf object but not 386

gosqlite._Cfunc_sqlite3_column_count: _cgo_a211002cfcb2_Cfunc_sqlite3_column_cou

nt: not defined

gosqlite._Cfunc_my_bind_text: _cgo_a211002cfcb2_Cfunc_my_bind_text: not defined

gosqlite._Cfunc_sqlite3_bind_parameter_count: _cgo_a211002cfcb2_Cfunc_sqlite3_bi

nd_parameter_count: not defined

gosqlite._Cfunc_sqlite3_step: _cgo_a211002cfcb2_Cfunc_sqlite3_step: not defined

gosqlite._Cfunc_sqlite3_column_bytes: _cgo_a211002cfcb2_Cfunc_sqlite3_column_byt

es: not defined

gosqlite._Cfunc_my_bind_blob: _cgo_a211002cfcb2_Cfunc_my_bind_blob: not defined

gosqlite._Cfunc_sqlite3_errmsg: _cgo_a211002cfcb2_Cfunc_sqlite3_errmsg: not defi

ned

gosqlite._Cfunc_sqlite3_reset: _cgo_a211002cfcb2_Cfunc_sqlite3_reset: not define

d

gosqlite._Cfunc_sqlite3_close: _cgo_a211002cfcb2_Cfunc_sqlite3_close: not define

d

gosqlite._Cfunc_sqlite3_column_blob: _cgo_a211002cfcb2_Cfunc_sqlite3_column_blob

: not defined

gosqlite._Cfunc_sqlite3_open_v2: _cgo_a211002cfcb2_Cfunc_sqlite3_open_v2: not de

fined

gosqlite._Cfunc_sqlite3_finalize: _cgo_a211002cfcb2_Cfunc_sqlite3_finalize: not

defined

gosqlite._Cfunc_free: _cgo_a211002cfcb2_Cfunc_free: not defined

gosqlite._Cfunc_sqlite3_prepare_v2: _cgo_a211002cfcb2_Cfunc_sqlite3_prepare_v2:

not defined

gosqlite._Cfunc_sqlite3_threadsafe: _cgo_a211002cfcb2_Cfunc_sqlite3_threadsafe:

not defined

.windynamic: _cgo_a211002cfcb2_Cfunc_sqlite3_errmsg(0): not defined

.windynamic: _cgo_a211002cfcb2_Cfunc_sqlite3_reset(0): not defined

too many errors

[peterGo, 2010-12-26 02:03:18]

http://codereview.appspot.com/3733046/diff/2001/src/cmd/ld/ldpe.c
File src/cmd/ld/ldpe.c (right):

http://codereview.appspot.com/3733046/diff/2001/src/cmd/ld/ldpe.c#newcode2
src/cmd/ld/ldpe.c:2: Copyright © 2010 Wei guangjing.
Use the Go standard copyright header.

Copyright, Contribution Guidelines
http://golang.org/doc/contribute.html#copyright

[vcc, 2010-12-26 02:21:22]

I test gosqlite with share lib, it works for me.

download sqlite3.dll from
http://www.sqlite.org/sqlite-dll-win32-x86-3070400.zip
unzip
dlltool -d sqlite3.def -l libsqlite3.a
make


2010/12/26 <go.peter.90@gmail.com>

> For this package,
>
> gosqlite: trivial sqlite3 binding for Go
> http://code.google.com/p/gosqlite/
>
> compiling this program,
>
> package main
>
> import (
>        "fmt"
>        sqlite "gosqlite"
> )
>
> func main() {
>        db, err := sqlite.Open("gosqlite.db")
>        fmt.Println(db, err)
> }
>
> fails when linking,
>
> $ 8l gosqlite.8
>
> adddynlib: unsupported binary format
>
> adddynlib: unsupported binary format
>
> p:/cgo/pkg/windows_386/gosqlite.a(sqlite.cgo2.o): elf object but not 386
>
> p:/cgo/pkg/windows_386/gosqlite.a(_cgo_export.o): elf object but not 386
>
> gosqlite._Cfunc_sqlite3_column_count:
> _cgo_a211002cfcb2_Cfunc_sqlite3_column_cou
>
> nt: not defined
>
> gosqlite._Cfunc_my_bind_text: _cgo_a211002cfcb2_Cfunc_my_bind_text: not
> defined
>
> gosqlite._Cfunc_sqlite3_bind_parameter_count:
> _cgo_a211002cfcb2_Cfunc_sqlite3_bi
>
> nd_parameter_count: not defined
>
> gosqlite._Cfunc_sqlite3_step: _cgo_a211002cfcb2_Cfunc_sqlite3_step: not
> defined
>
> gosqlite._Cfunc_sqlite3_column_bytes:
> _cgo_a211002cfcb2_Cfunc_sqlite3_column_byt
>
> es: not defined
>
> gosqlite._Cfunc_my_bind_blob: _cgo_a211002cfcb2_Cfunc_my_bind_blob: not
> defined
>
> gosqlite._Cfunc_sqlite3_errmsg: _cgo_a211002cfcb2_Cfunc_sqlite3_errmsg:
> not defi
>
> ned
>
> gosqlite._Cfunc_sqlite3_reset: _cgo_a211002cfcb2_Cfunc_sqlite3_reset:
> not define
>
> d
>
> gosqlite._Cfunc_sqlite3_close: _cgo_a211002cfcb2_Cfunc_sqlite3_close:
> not define
>
> d
>
> gosqlite._Cfunc_sqlite3_column_blob:
> _cgo_a211002cfcb2_Cfunc_sqlite3_column_blob
>
> : not defined
>
> gosqlite._Cfunc_sqlite3_open_v2:
> _cgo_a211002cfcb2_Cfunc_sqlite3_open_v2: not de
>
> fined
>
> gosqlite._Cfunc_sqlite3_finalize:
> _cgo_a211002cfcb2_Cfunc_sqlite3_finalize: not
>
> defined
>
> gosqlite._Cfunc_free: _cgo_a211002cfcb2_Cfunc_free: not defined
>
> gosqlite._Cfunc_sqlite3_prepare_v2:
> _cgo_a211002cfcb2_Cfunc_sqlite3_prepare_v2:
>
> not defined
>
> gosqlite._Cfunc_sqlite3_threadsafe:
> _cgo_a211002cfcb2_Cfunc_sqlite3_threadsafe:
>
> not defined
>
> .windynamic: _cgo_a211002cfcb2_Cfunc_sqlite3_errmsg(0): not defined
>
> .windynamic: _cgo_a211002cfcb2_Cfunc_sqlite3_reset(0): not defined
>
> too many errors
>
>
> http://codereview.appspot.com/3733046/
>

[czaplinski, 2010-12-26 14:39:30]

http://codereview.appspot.com/3733046/diff/2001/src/cmd/ld/ldpe.c
File src/cmd/ld/ldpe.c (right):

http://codereview.appspot.com/3733046/diff/2001/src/cmd/ld/ldpe.c#newcode382
src/cmd/ld/ldpe.c:382: PeSect *sect;
(minor) looks like having some slight indentation issues till the end of this
func.

[vcc, 2011-01-05 05:15:39]

Please take another look.

http://codereview.appspot.com/3733046/diff/2001/src/cmd/ld/ldpe.c
File src/cmd/ld/ldpe.c (right):

http://codereview.appspot.com/3733046/diff/2001/src/cmd/ld/ldpe.c#newcode2
src/cmd/ld/ldpe.c:2: Copyright © 2010 Wei guangjing.
On 2010/12/26 02:03:18, peterGo wrote:
> Use the Go standard copyright header.
> 
> Copyright, Contribution Guidelines
> http://golang.org/doc/contribute.html#copyright

Done.

http://codereview.appspot.com/3733046/diff/2001/src/cmd/ld/ldpe.c#newcode382
src/cmd/ld/ldpe.c:382: PeSect *sect;
On 2010/12/26 14:39:30, czaplinski wrote:
> (minor) looks like having some slight indentation issues till the end of this
> func.

Done.

[vcc, 2011-01-08 07:41:55]

cgocallback works now, that make misc/cgo/life works, please take another look.

[rsc, 2011-01-19 20:04:19]

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

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/data.c#newcode244
src/cmd/ld/data.c:244: if(HEADTYPE == 10) { // Windows PE
if(thechar == '8' && HEADTYPE == 10) {

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/data.c#newcode252
src/cmd/ld/data.c:252: if(r->sym->plt == -2 ) { // make dynimport JMP table for
PE object files.
s/ )/)/

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/data.c#newcode263
src/cmd/ld/data.c:263: } else if(r->sym->plt >=0) {
s/>=/>= /

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/ldpe.c
File src/cmd/ld/ldpe.c (right):

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/ldpe.c#newcode150
src/cmd/ld/ldpe.c:150: // TODO riase error if found .cormeta .rsrc
s/riase/return/

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/lib.c#newcode409
src/cmd/ld/lib.c:409: if(c1 == 0x4c && c2 == 0x01 || c1 == 0x64 && c2 == 0x86 ||
c1 == 0x00 && c2 == 0x00) {
c1 == 0 && c2 == 0 is a pretty common case i would imagine.
is it really PE?  is there some other check that we should make?

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/386/asm.s
File src/pkg/runtime/386/asm.s (right):

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/386/asm.s#ne...
src/pkg/runtime/386/asm.s:22: CALL	AX
The JMP ok that used to follow this line is important.
If we called initcgo, then we should not go messing with the LDT ourselves.
Please put it back.

Can do

CMPL runtime·iswindows(SB), $0
JEQ ok

if need to set up %gs on windows;

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/Makefile
File src/pkg/runtime/cgo/Makefile (right):

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/Makefile...
src/pkg/runtime/cgo/Makefile:32: ifeq ($(GOOS),windows)
i'd be happy to set

CGO_LDFLAGS=-lm -lpthread

unconditionally, so you can get rid of the ifeq.

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/windows_...
File src/pkg/runtime/cgo/windows_386.c (right):

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/windows_...
src/pkg/runtime/cgo/windows_386.c:33: void* tls0;
s/void* /void */

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/windows_...
src/pkg/runtime/cgo/windows_386.c:51: "MOVL %0, %%fs:0x2c\n"	// MOVL tls0,
0x2c(FS)
s/MOVL/movl/ - this is gcc syntax
next line too

i'm a little confused about overwriting the TLS section though.
that seems wrong - isn't windows expecting its own stuff there?

[vcc, 2011-01-20 13:22:58]

PTAL

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

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/data.c#newcode244
src/cmd/ld/data.c:244: if(HEADTYPE == 10) { // Windows PE
On 2011/01/19 20:04:19, rsc wrote:
> if(thechar == '8' && HEADTYPE == 10) {

Done.

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/data.c#newcode252
src/cmd/ld/data.c:252: if(r->sym->plt == -2 ) { // make dynimport JMP table for
PE object files.
On 2011/01/19 20:04:19, rsc wrote:
> s/ )/)/

Done.

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/data.c#newcode263
src/cmd/ld/data.c:263: } else if(r->sym->plt >=0) {
On 2011/01/19 20:04:19, rsc wrote:
> s/>=/>= /

Done.

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/ldpe.c
File src/cmd/ld/ldpe.c (right):

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/ldpe.c#newcode150
src/cmd/ld/ldpe.c:150: // TODO riase error if found .cormeta .rsrc
On 2011/01/19 20:04:19, rsc wrote:
> s/riase/return/

Done.

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

http://codereview.appspot.com/3733046/diff/21001/src/cmd/ld/lib.c#newcode409
src/cmd/ld/lib.c:409: if(c1 == 0x4c && c2 == 0x01 || c1 == 0x64 && c2 == 0x86 ||
c1 == 0x00 && c2 == 0x00) {
On 2011/01/19 20:04:19, rsc wrote:
> c1 == 0 && c2 == 0 is a pretty common case i would imagine.
> is it really PE?  is there some other check that we should make?

There are CPU types, 0x4c01 is 386 or later processors and compatible
processors, 0x6486 is amd64, 0x0000 is unknown, also have many other CPU types.
I think just use 386 and amd64 is ok now, so remove unknown.

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/386/asm.s
File src/pkg/runtime/386/asm.s (right):

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/386/asm.s#ne...
src/pkg/runtime/386/asm.s:22: CALL	AX
On 2011/01/19 20:04:19, rsc wrote:
> The JMP ok that used to follow this line is important.
> If we called initcgo, then we should not go messing with the LDT ourselves.
> Please put it back.
> 
> Can do
> 
> CMPL runtime·iswindows(SB), $0
> JEQ ok
> 
> if need to set up %gs on windows;
> 
> 

Done.

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/Makefile
File src/pkg/runtime/cgo/Makefile (right):

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/Makefile...
src/pkg/runtime/cgo/Makefile:32: ifeq ($(GOOS),windows)
On 2011/01/19 20:04:19, rsc wrote:
> i'd be happy to set
> 
> CGO_LDFLAGS=-lm -lpthread
> 
> unconditionally, so you can get rid of the ifeq.

windows don't have pthread, use -mthreads flag, not -lpthread.

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/windows_...
File src/pkg/runtime/cgo/windows_386.c (right):

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/windows_...
src/pkg/runtime/cgo/windows_386.c:33: void* tls0;
On 2011/01/19 20:04:19, rsc wrote:
> s/void* /void */

Done.

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/windows_...
src/pkg/runtime/cgo/windows_386.c:33: void* tls0;
On 2011/01/19 20:04:19, rsc wrote:
> s/void* /void */

Done.

http://codereview.appspot.com/3733046/diff/21001/src/pkg/runtime/cgo/windows_...
src/pkg/runtime/cgo/windows_386.c:51: "MOVL %0, %%fs:0x2c\n"	// MOVL tls0,
0x2c(FS)
On 2011/01/19 20:04:19, rsc wrote:
> s/MOVL/movl/ - this is gcc syntax

Done.

> next line too
> 
> i'm a little confused about overwriting the TLS section though.
> that seems wrong - isn't windows expecting its own stuff there?

We need set value in TLS slot.

[rsc, 2011-01-20 13:30:37]

looks good; one nit to make it work on non-windows

http://codereview.appspot.com/3733046/diff/34001/src/pkg/runtime/windows/386/...
File src/pkg/runtime/windows/386/rt0.s (right):

http://codereview.appspot.com/3733046/diff/34001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/rt0.s:8: DATA  runtime·iswindows(SB)/4, $1
need a definition in runtime.h too; look for isplan9.

[vcc, 2011-01-20 13:39:19]

PTAL

http://codereview.appspot.com/3733046/diff/34001/src/pkg/runtime/windows/386/...
File src/pkg/runtime/windows/386/rt0.s (right):

http://codereview.appspot.com/3733046/diff/34001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/rt0.s:8: DATA  runtime·iswindows(SB)/4, $1
On 2011/01/20 13:30:37, rsc wrote:
> need a definition in runtime.h too; look for isplan9.

Done.

[rsc, 2011-01-20 14:20:56]

looks good but please hg sync and hg mail.
doesn't apply cleanly with your most recent 6l changes.

[vcc, 2011-01-20 14:36:50]

On 2011/01/20 14:20:56, rsc wrote:
> looks good but please hg sync and hg mail.
> doesn't apply cleanly with your most recent 6l changes.

Done hg sync and hg mail, please take another look.

Thanks for review.

[rsc, 2011-01-20 15:12:12]

LGTM

[rsc, 2011-01-20 15:22:23]

*** Submitted as 5cff7a770e6d ***

cgo: windows/386 port

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

Committer: Russ Cox <rsc@golang.org>