code review 9080043: cmd/ld: fix syms that are both cgo_import_static & cgo_...

cmd/ld: fix syms that are both cgo_import_static & cgo_import_dynamic

This is needed for SWIG when linking in internal mode.  In
internal mode if a symbol was cgo_import_static we used to
forget that it was also cgo_import_dynamic.

[iant, 2013-05-01 14:27:13]

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

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

[rsc, 2013-05-01 15:46:49]

LGTM

[r, 2013-05-01 21:26:33]

LGTM

[iant, 2013-05-01 21:30:29]

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

cmd/ld: fix syms that are both cgo_import_static & cgo_import_dynamic

This is needed for SWIG when linking in internal mode.  In
internal mode if a symbol was cgo_import_static we used to
forget that it was also cgo_import_dynamic.

R=rsc, r
CC=golang-dev
https://codereview.appspot.com/9080043

[brainman, 2013-05-02 00:56:53]

It crashes in https://code.google.com/p/go/source/browse/src/cmd/ld/pe.c#201,
because s->dynimplib is nil. Before this change the code was executed for

#pragma dynimport runtime·CloseHandle CloseHandle "kernel32.dll"

lines only. We are building "import table" in pe executable header. Windows
program loader will use these to find dll functions our executable will need
during its run.

These change also adds these lines

#pragma cgo_import_static _cgo_db37227116aa_C2func_fopen

into our consideration. I do not understand the purpose of cgo_import_static,
but these shouldn't really need to be considered here.

Need advice. What should I do? How should I distinguish these two now, so I can
ignore the later?

Thank you.

Alex

[iant, 2013-05-02 01:53:38]

On Wed, May 1, 2013 at 5:56 PM,  <alex.brainman@gmail.com> wrote:
> It crashes in
> https://code.google.com/p/go/source/browse/src/cmd/ld/pe.c#201, because
> s->dynimplib is nil. Before this change the code was executed for
>
> #pragma dynimport runtime·CloseHandle CloseHandle "kernel32.dll"
>
> lines only. We are building "import table" in pe executable header.
> Windows program loader will use these to find dll functions our
> executable will need during its run.
>
> These change also adds these lines
>
> #pragma cgo_import_static _cgo_db37227116aa_C2func_fopen
>
> into our consideration. I do not understand the purpose of
> cgo_import_static, but these shouldn't really need to be considered
> here.
>
> Need advice. What should I do? How should I distinguish these two now,
> so I can ignore the later?

Does it work if you change line 339 of ld/lib.c, which is currently this:
				if(s->extname != nil && s->cgoexport == 0) {
to this
				if(s->extname != nil && s->dynimplib != nil && s->cgoexport == 0) {
?

#pragma cgo_import_static should only be relevant in external linking
mode.  #pragma dynimport is the same as #pragma cgo_import_dynamic,
and should only be relevant in internal linking mode.

This change looks for cases where a symbol is both #pragma
cgo_import_static and #pragma cgo_import_dynamic.  Before this change,
when using internal linking mode, the fact that the symbol was
cgo_import_dynamic was being lost.  It's possible that my test for
whether the symbol was both cgo_import_static and cgo_import_dynamic
was wrong; my proposed change tightens that test.

If that doesn't help I don't know what is wrong but I hope this helps
you understand the issue further.

Ian

[brainman, 2013-05-02 02:34:22]

On 2013/05/02 01:53:38, iant wrote:
> ... 
> Does it work if you change line 339 of ld/lib.c, which is currently this:
> 				if(s->extname != nil && s->cgoexport == 0) {
> to this
> 				if(s->extname != nil && s->dynimplib != nil && s->cgoexport == 0) {
> ?
> 

It does work.

> #pragma cgo_import_static should only be relevant in external linking
> mode.  #pragma dynimport is the same as #pragma cgo_import_dynamic,
> and should only be relevant in internal linking mode.
> 
> This change looks for cases where a symbol is both #pragma
> cgo_import_static and #pragma cgo_import_dynamic.  Before this change,
> when using internal linking mode, the fact that the symbol was
> cgo_import_dynamic was being lost.  It's possible that my test for
> whether the symbol was both cgo_import_static and cgo_import_dynamic
> was wrong; my proposed change tightens that test.
> 

Thank you for explaining that. It clears my mind a little :-). Would you like me
to send the change as I understand it, or will you do it yourself?

Alex

[iant, 2013-05-02 02:48:18]

On Wed, May 1, 2013 at 7:34 PM,  <alex.brainman@gmail.com> wrote:
>
> Thank you for explaining that. It clears my mind a little :-). Would you
> like me to send the change as I understand it, or will you do it
> yourself?

Please go ahead if you have it ready.

Thanks.

Ian

[brainman, 2013-05-02 03:01:12]

https://codereview.appspot.com/9110044/

Alex