code review 2166041: 8l : add dynimport to import table in Windows PE, initi...

8l : add dynimport to import table in Windows PE, initial make cgo dll work.

[vcc, 2010-09-06 14:55:47]

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

I'd like you to review this change.

[brainman, 2010-09-06 23:50:49]

> I'd like you to review this change.

The code looks good. But I can't see how would I use this facility. Perhaps, you
can give me an example of how this new functionality can be used.

Alex

[vcc, 2010-09-07 14:51:07]

add export variable support, please take another look.

for run cgo test it, need more CL:
1976045 <http://codereview.appspot.com/1976045/>
1910041 <http://codereview.appspot.com/1910041/>
2068041 <http://codereview.appspot.com/2068041/>

I test with some sqlite3 go bind, look work for me.
and for $GOROOT/misc/cgo/stdio example, it look like mingw gcc can't export
stdout/stderr symbol in dll, so not work.

Wei guangjing

2010/9/7 <alex.brainman@gmail.com>

> I'd like you to review this change.
>>
>
> The code looks good. But I can't see how would I use this facility.
> Perhaps, you can give me an example of how this new functionality can be
> used.
>
> Alex
>
>
> http://codereview.appspot.com/2166041/
>

[Joe Poirier, 2010-09-08 17:12:15]

On Tue, Sep 7, 2010 at 9:51 AM, Wei guangjing <vcc.163@gmail.com> wrote:
> add export variable support, please take another look.
>
> for run cgo test it, need more CL:
> 1976045
> 1910041
> 2068041
>
> I test with some sqlite3 go bind, look work for me.
> and for $GOROOT/misc/cgo/stdio example, it look like mingw gcc can't export
> stdout/stderr symbol in dll, so not work.
>
> Wei guangjing
>

Hi Wei guangjing,

I applied 1976045, 1910041, 2068041, 2166041 to release 2010-09-06
but when I build my go file with cgo I get a parameter incorrect
error. The error occurs whether I build my file or any of the others in
go/misc/cgo.

What version of Go did you use and were there any extra hacks that you
had to use to get it to work?

Thanks,
joe

[vcc, 2010-09-18 03:45:36]

Add dynimport kernel32.dll fundctions into runtime, so don't need get process
address on init, please take another look.

[brainman, 2010-09-18 06:20:46]

On 2010/09/18 03:45:36, vcc wrote:
> Add dynimport kernel32.dll fundctions into runtime, so don't need get process
> address on init, please take another look.

Why didn't you handled all of them in windows_goargs():
...
gcl = get_proc_addr("kernel32.dll", "GetCommandLineW");
clta = get_proc_addr("shell32.dll", "CommandLineToArgvW");
ges = get_proc_addr("kernel32.dll", "GetEnvironmentStringsW");
fes = get_proc_addr("kernel32.dll", "FreeEnvironmentStringsW");
...
?

Also, while, I admit, there is some merit to this change, to allow windows
loader do the dirty work for us at start up, I don't see any advantages but some
simplification in runtime.

Alex

[vcc, 2010-09-22 02:34:35]

handle in windows_goargs, please take another look.

On 2010/09/18 06:20:46, brainman wrote:
> Why didn't you handled all of them in windows_goargs():
> ...
> gcl = get_proc_addr("kernel32.dll", "GetCommandLineW");
> clta = get_proc_addr("shell32.dll", "CommandLineToArgvW");
> ges = get_proc_addr("kernel32.dll", "GetEnvironmentStringsW");
> fes = get_proc_addr("kernel32.dll", "FreeEnvironmentStringsW");

done.

[brainman, 2010-09-22 02:59:23]

On 2010/09/22 02:34:35, vcc wrote:
> handle in windows_goargs, please take another look.
> 

Russ, do you want us to go ahead with this?

[rsc, 2010-09-22 03:14:48]

It seems like something we'll need eventually.
Thanks for figuring out how to do it, Wei!
The only comments I have are on the C code,
which does not match the surrounding code.
For example, all variables should be declared at
the top of the function, without initializations.
I'd also like to avoid the big fixed-size array
if possible.  Use a dynamically allocated structure.
It might help to define a real data structure instead
of using parallel arrays.  Please use strcmp==0
rather than !strcmp.

Russ

[vcc, 2010-09-22 07:51:13]

Done, please take another look.

On 2010/09/22 03:14:48, rsc wrote:
> It seems like something we'll need eventually.
> Thanks for figuring out how to do it, Wei!
> The only comments I have are on the C code,
> which does not match the surrounding code.
> For example, all variables should be declared at
> the top of the function, without initializations.
> I'd also like to avoid the big fixed-size array
> if possible.  Use a dynamically allocated structure.
> It might help to define a real data structure instead
> of using parallel arrays.  Please use strcmp==0
> rather than !strcmp.
> 
> Russ
>

[rsc1, 2010-09-22 18:46:43]

looks pretty good; a few more small things

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode136
src/cmd/ld/pe.c:136: struct Imp {
please move these above the add_import_table function so they're at top level.
the usual way to declare a type in this code is

typedef struct Imp Imp;
struct Imp {
    ...
};

typedef struct Dll Dll;
struct Dll {
    ...
};

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode156
src/cmd/ld/pe.c:156: if(!s->reachable)
i'd suggest making this if(!s->reachable || !s->dynimpname)
and then you can drop a level of indentation in the rest of the loop

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode161
src/cmd/ld/pe.c:161: d = dr;
for(d = dr; d != nil; d = d->next) {

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode168
src/cmd/ld/pe.c:168: d->count ++;
drop space before ++

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode175
src/cmd/ld/pe.c:175: if(found == 0) {
if(d == nil)
would work here too

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode214
src/cmd/ld/pe.c:214: d = dr;
for(d = dr; d != nil; d = d->next) {

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode236
src/cmd/ld/pe.c:236: d = dr;
for(d = dr; d != nil; d = d->next) {
etc
every while loop in this function should be a for loop

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode284
src/cmd/ld/pe.c:284: cput(0xff);cput(0x25);
one statement per line please

[vcc, 2010-09-23 09:02:57]

All done, please take another look.

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode136
src/cmd/ld/pe.c:136: struct Imp {
On 2010/09/22 18:46:43, rsc1 wrote:
> please move these above the add_import_table function so they're at top level.
> the usual way to declare a type in this code is
> 
> typedef struct Imp Imp;
> struct Imp {
>     ...
> };
> 
> typedef struct Dll Dll;
> struct Dll {
>     ...
> };
> 
> 

Done.

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode156
src/cmd/ld/pe.c:156: if(!s->reachable)
On 2010/09/22 18:46:43, rsc1 wrote:
> i'd suggest making this if(!s->reachable || !s->dynimpname)
> and then you can drop a level of indentation in the rest of the loop
> 

Done.

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode161
src/cmd/ld/pe.c:161: d = dr;
On 2010/09/22 18:46:43, rsc1 wrote:
> for(d = dr; d != nil; d = d->next) {
> 

Done.

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode168
src/cmd/ld/pe.c:168: d->count ++;
On 2010/09/22 18:46:43, rsc1 wrote:
> drop space before ++
> 

Done.

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode175
src/cmd/ld/pe.c:175: if(found == 0) {
On 2010/09/22 18:46:43, rsc1 wrote:
> if(d == nil)
> would work here too
> 

Done.

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode214
src/cmd/ld/pe.c:214: d = dr;
On 2010/09/22 18:46:43, rsc1 wrote:
> for(d = dr; d != nil; d = d->next) {
> 

Done.

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode236
src/cmd/ld/pe.c:236: d = dr;
On 2010/09/22 18:46:43, rsc1 wrote:
> for(d = dr; d != nil; d = d->next) {
> etc
> every while loop in this function should be a for loop
> 

Done.

http://codereview.appspot.com/2166041/diff/27001/src/cmd/ld/pe.c#newcode284
src/cmd/ld/pe.c:284: cput(0xff);cput(0x25);
On 2010/09/22 18:46:43, rsc1 wrote:
> one statement per line please
> 

Done.

[brainman, 2010-09-28 05:49:00]

Russ,

I need your help / opinion here.

The way windows runtime loader works is, it gets list of function names we
require along with a pointer to an array of 32 bit values to put their addresses
to. It walks along the list of names and fill array of addresses one at the
time. Important bit here, is that all function addresses will have to be in one
single block of memory.

I look, and all our dynimport variables are in data segment, but they are spread
about. Is it possible (is it easy enough <g>), to layout them in such way, that
they all are in a single continues block?

Because, alternatively, as Wei suggest, we must have "double redirection": he
creates block for windows loader to place actual addresses, then creates a table
of JMP commands using this data to jump to our destination, and finally,
rewrites all dynimport variables with address of JMP commands.

Another alternative is to avoid JMP table, and refer from dynimport straight
into table of actual addresses, but then dynimport variables have to be
dereferenced once inside our runtime, they won't be real function addresses on
startup.

What do you think?

Alex

[brainman, 2010-09-28 05:53:31]

Lots of comments, but, most importantly, it works, which is a always a good
start!

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode161
src/cmd/ld/pe.c:161: for(s = hash[i]; s != S; s = s->link) {
Shouldn't this be indented?

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode164
src/cmd/ld/pe.c:164: fn++;
What about this instead:

for(d = dr; d != nil; d = d->next)
	if(strcmp(d->name,s->dynimplib) == 0)
		break;
if(d == nil) {
	d = mal(sizeof *d);
	d->name = s->dynimplib;
	d->ms = 0;
	d->count = 0;
	d->next = dr;
	dr = d;
	dn++;
	size += strlen(s->dynimplib)+1;
}
m = mal(sizeof *m);
m->s = s;
m->next = d->ms;
d->ms = m;
d->count++;
fn++;
size += strlen(s->dynimpname)+2+1;

? But I'm not fussy either way.

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode206
src/cmd/ld/pe.c:206: 
I'm confused by all your variables, I don't think you need so many (boff,
last_fn, ...). Also, all your size and offset calculations are bit cryptic. I
could be wrong, but, perhaps, if you reorder data (have JMP table, then all
strings, then FirstThunk and OriginalFirstThunk, and then dll descriptors,
everything will work out better.

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode224
src/cmd/ld/pe.c:224: 
Delete blank line. It is the end of output in the loop.

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode231
src/cmd/ld/pe.c:231: // put OriginalFirstThunk
Perhaps:

// put OriginalFirstThunk
o = last_name_off;
for(d = dr; d != nil; d = d->next) {
	for(m = d->ms; m != nil; m = m->next) {
		lputl(o);
		o += 2 + strlen(m->s->dynimpname) + 1;
	}
	lputl(0);
}
// put FirstThunk
o = last_name_off;
for(d = dr; d != nil; d = d->next) {
	for(m = d->ms; m != nil; m = m->next) {
		m->va = PEBASE + va + cpos() - poff;
		lputl(o);
		o += 2 + strlen(m->s->dynimpname) + 1;
	}
	lputl(0);
}

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode244
src/cmd/ld/pe.c:244: m->va = PEBASE + va + cpos() - poff;
You pre calculate all other offsets not this one. Why?

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode257
src/cmd/ld/pe.c:257: strput(m->s->dynimpname);
According to this article
http://www.woodmann.com/yates/SystemInformation/iat.html (and all .exe files
I've seen), all function names should be padded with 0 to even address. It seems
to work for you, but ...

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode260
src/cmd/ld/pe.c:260: strnput("", isect->SizeOfRawData - size);
Shouldn't this JMP table be part of current section (be included in the size)?

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode264
src/cmd/ld/pe.c:264: m->vb = PEBASE + va + cpos() - poff;
You pre calculate all other offsets not this one. Why?

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode278
src/cmd/ld/pe.c:278: if(strstr(s->name,"._Cvar_")) {
I don't understand what is happening here. None of our functions have "._Cvar_"
in their name.

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (left):

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:29: get_proc_addr2(byte *base, byte *name)
get_proc_addr is not used any more, please delete it.

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:33: #pragma dynimport CreateEvent CreateEventA
"kernel32.dll"
I don't like you're renaming functions here, because you're using both *A and *W
versions. I suggest, either stick to one type *A or *W, or keep names as is, so
we can see what is happening right in context.

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:38: void *CreateEvent;
Don't make variables global, unless need to.

[Joe Poirier, 2010-09-28 11:01:59]

On Tue, Sep 28, 2010 at 12:49 AM,  <alex.brainman@gmail.com> wrote:
> Russ,
>
> I need your help / opinion here.
>
> The way windows runtime loader works is, it gets list of function names
> we require along with a pointer to an array of 32 bit values to put
> their addresses to. It walks along the list of names and fill array of
> addresses one at the time. Important bit here, is that all function
> addresses will have to be in one single block of memory.
>
> I look, and all our dynimport variables are in data segment, but they
> are spread about. Is it possible (is it easy enough <g>), to layout them
> in such way, that they all are in a single continues block?
>
> Because, alternatively, as Wei suggest, we must have "double
> redirection": he creates block for windows loader to place actual
> addresses, then creates a table of JMP commands using this data to jump
> to our destination, and finally, rewrites all dynimport variables with
> address of JMP commands.
>
> Another alternative is to avoid JMP table, and refer from dynimport
> straight into table of actual addresses, but then dynimport variables
> have to be dereferenced once inside our runtime, they won't be real
> function addresses on startup.
>
> What do you think?
>
> Alex
>

It may be worth while to look at tcc's tccpe.c file.

http://download.savannah.nongnu.org/releases/tinycc/tcc-0.9.25.tar.bz2

-joe

[rsc, 2010-09-28 14:10:56]

> The way windows runtime loader works is, it gets list of function names
> we require along with a pointer to an array of 32 bit values to put
> their addresses to. It walks along the list of names and fill array of
> addresses one at the time. Important bit here, is that all function
> addresses will have to be in one single block of memory.
>
> I look, and all our dynimport variables are in data segment, but they
> are spread about. Is it possible (is it easy enough <g>), to layout them
> in such way, that they all are in a single continues block?

Sure.  If you look at how the data gets laid out (dodata)
you could introduce a pass at the beginning to lay out just
the dynimport variables and also to remember the start
and end locations and their order.

Russ

[vcc, 2010-09-28 15:28:43]

On 2010/09/28 11:01:59, Joe Poirier wrote:
> It may be worth while to look at tcc's tccpe.c file.
> 
> http://download.savannah.nongnu.org/releases/tinycc/tcc-0.9.25.tar.bz2
> 
> -joe
> 

tinycc also use JMP table.

Wei guangjing

[brainman, 2010-09-28 23:19:49]

> ...  If you look at how the data gets laid out (dodata)
> you could introduce a pass at the beginning to lay out just
> the dynimport variables and also to remember the start
> and end locations and their order.

Thank you Russ. Wei, would you like to have a go at it?

[brainman, 2010-09-28 23:38:47]

Sorry, one more.

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:16: #pragma dynimport VirtualFree VirtualFree
"kernel32.dll"
These two - VirtualAlloc and VirtualFree - could go to windows/mem.c, where they
are used.

[vcc, 2010-09-29 03:13:47]

2010/9/29 <alex.brainman@gmail.com>

> ...  If you look at how the data gets laid out (dodata)
>>
>> you could introduce a pass at the beginning to lay out just
>> the dynimport variables and also to remember the start
>> and end locations and their order.
>>
>
> Thank you Russ. Wei, would you like to have a go at it?
>
>
I will look into it.

>
> http://codereview.appspot.com/2166041/
>

[vcc, 2010-09-30 05:15:27]

For avoid JMP table, we need can direct call dynimport function in Go first,
otherwise we need change thing like stdcall(GetCommandLine, 0) to
stdcall(*GetCommandLine, 0), it add one *addr operation and make code
complex, I don't see any benefit.

I suggest use JMP table right now, JMP table is ok, mingw gcc also use it.

I'd like to suggest use compiler handle the details for stdcall, for
example, add stdcall support to #pragma, like this:
#pragma dynimport GetCommandLine GetCommandLineW  "kernel32.dll" stdcall
so the compiler know this is a stdcall function, and use stdcall way to push
args and make stack alloc, then we can direct call it from Go.

Maybe also for callbacks(I saw Alex implement it in another CL), have
“#pragma funcname callback” would be help.

Wei guangjing

2010/9/29 Wei guangjing <vcc.163@gmail.com>

> 2010/9/29 <alex.brainman@gmail.com>
>
>  ...  If you look at how the data gets laid out (dodata)
>>>
>>> you could introduce a pass at the beginning to lay out just
>>> the dynimport variables and also to remember the start
>>> and end locations and their order.
>>>
>>
>> Thank you Russ. Wei, would you like to have a go at it?
>>
>>
> I will look into it.
>
>>
>> http://codereview.appspot.com/2166041/
>>
>
>

[brainman, 2010-09-30 05:44:03]

> For avoid JMP table, we need can direct call dynimport function in Go first,
> otherwise we need change thing like stdcall(GetCommandLine, 0) to
> stdcall(*GetCommandLine, 0), ...

Not true. If you read this explanation
http://www.woodmann.com/yates/SystemInformation/iat.html you will see that if we
could layout all our dynimport variables one after the other in one single block
(IAT as it is referred in the article), os loader will write DLL function
addresses into them, and that is what we need.

> ... it add one *addr operation and make code
> complex, I don't see any benefit.

The benefit is exactly what you said. Assuming it is easy enough to collate all
dynimport variables (as Russ suggested), the code will be simpler, because you
don't need JMP table. Also, during runtime, all syscalls will run directly, not
via 2 dereferences - first to jump to JMP table, and second to execute JMP
dereferencing memory pointed by JMP instruction.

> I suggest use JMP table right now, JMP table is ok, ...

If you insist, but I hope we can do better.

> I'd like to suggest use compiler handle the details for stdcall, for
> example, add stdcall support to #pragma, like this:
> #pragma dynimport GetCommandLine GetCommandLineW  "kernel32.dll" stdcall
> so the compiler know this is a stdcall function, and use stdcall way to push
> args and make stack alloc, then we can direct call it from Go.

I don't care for that. Why complicate compiler? What is wrong with the way it
works now?

Alex

[vcc, 2010-09-30 09:30:38]

2010/9/30 <alex.brainman@gmail.com>

> ...
> The benefit is exactly what you said. Assuming it is easy enough to
> collate all dynimport variables (as Russ suggested), the code will be
> simpler, because you don't need JMP table. Also, during runtime, all
> syscalls will run directly, not via 2 dereferences - first to jump to
> JMP table, and second to execute JMP dereferencing memory pointed by JMP
> instruction.
>

You are right, we can do it.

>
> ...
>
>  I'd like to suggest use compiler handle the details for stdcall, for
>> example, add stdcall support to #pragma, like this:
>> #pragma dynimport GetCommandLine GetCommandLineW  "kernel32.dll"
>>
> stdcall
>
>> so the compiler know this is a stdcall function, and use stdcall way
>>
> to push
>
>> args and make stack alloc, then we can direct call it from Go.
>>
>
> I don't care for that. Why complicate compiler? What is wrong with the
> way it works now?
>
>
Compiler do it can avoid helper calls and duplication args in stack.


> http://codereview.appspot.com/2166041/
>
>

[brainman, 2010-09-30 23:05:23]

On 2010/09/30 09:30:38, vcc wrote:
> > ... I'd like to suggest use compiler handle the details for stdcall, for
> >> example, add stdcall support to #pragma, like this:
> >> #pragma dynimport GetCommandLine GetCommandLineW  "kernel32.dll"
> >>
> > stdcall
> >
> >> so the compiler know this is a stdcall function, and use stdcall way
> >>
> > to push
> >
> >> args and make stack alloc, then we can direct call it from Go.
> >>
> >
> > I don't care for that. Why complicate compiler? What is wrong with the
> > way it works now?
> >
> >
> Compiler do it can avoid helper calls and duplication args in stack.

You would have to do more then that to convince me. Example or something?

Alex

[vcc, 2010-10-01 03:54:33]

Don't use JMP table now, thanks Alex and Russ, please take another look.

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode161
src/cmd/ld/pe.c:161: for(s = hash[i]; s != S; s = s->link) {
On 2010/09/28 05:53:38, brainman wrote:
> Shouldn't this be indented?

same style as pass.c..., and I like this style.

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode224
src/cmd/ld/pe.c:224: 
On 2010/09/28 05:53:38, brainman wrote:
> Delete blank line. It is the end of output in the loop.

Done.

http://codereview.appspot.com/2166041/diff/32001/src/cmd/ld/pe.c#newcode231
src/cmd/ld/pe.c:231: // put OriginalFirstThunk
On 2010/09/28 05:53:38, brainman wrote:
> Perhaps:
> 
> // put OriginalFirstThunk
> o = last_name_off;
> for(d = dr; d != nil; d = d->next) {
> 	for(m = d->ms; m != nil; m = m->next) {
> 		lputl(o);
> 		o += 2 + strlen(m->s->dynimpname) + 1;
> 	}
> 	lputl(0);
> }
> // put FirstThunk
> o = last_name_off;
> for(d = dr; d != nil; d = d->next) {
> 	for(m = d->ms; m != nil; m = m->next) {
> 		m->va = PEBASE + va + cpos() - poff;
> 		lputl(o);
> 		o += 2 + strlen(m->s->dynimpname) + 1;
> 	}
> 	lputl(0);
> }

Done.

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:16: #pragma dynimport VirtualFree VirtualFree
"kernel32.dll"
On 2010/09/28 23:38:47, brainman wrote:
> These two - VirtualAlloc and VirtualFree - could go to windows/mem.c, where
they
> are used.

Done.

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:33: #pragma dynimport CreateEvent CreateEventA
"kernel32.dll"
On 2010/09/28 05:53:38, brainman wrote:
> I don't like you're renaming functions here, because you're using both *A and
*W
> versions. I suggest, either stick to one type *A or *W, or keep names as is,
so
> we can see what is happening right in context.

Need *A or *W, otherwise loader will rise can't found error.

http://codereview.appspot.com/2166041/diff/32001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:38: void *CreateEvent;
On 2010/09/28 05:53:38, brainman wrote:
> Don't make variables global, unless need to.

Need.

[brainman, 2010-10-05 02:00:12]

On 2010/10/01 03:54:33, vcc wrote:
> Don't use JMP table now, ...

Good work. But it looks too complicated to me. Here is my suggestion for
8l/pass.c:

diff -r cc2fee944133 src/cmd/8l/pass.c
--- a/src/cmd/8l/pass.c	Tue Oct 05 11:02:15 2010 +1100
+++ b/src/cmd/8l/pass.c	Tue Oct 05 12:37:13 2010 +1100
@@ -30,6 +30,7 @@
 
 #include	"l.h"
 #include	"../ld/lib.h"
+#include	"../ld/pe.h"
 
 // see ../../pkg/runtime/proc.c:/StackGuard
 enum
@@ -92,6 +93,8 @@
 	for(s = hash[i]; s != S; s = s->link) {
 		if(!s->reachable)
 			continue;
+		if(s->dynimpname && HEADTYPE == 10)
+			continue;
 		if(s->type != SDATA)
 		if(s->type != SBSS)
 			continue;
@@ -112,6 +115,8 @@
 	/* allocate the rest of the data */
 	for(i=0; i<NHASH; i++)
 	for(s = hash[i]; s != S; s = s->link) {
+		if(s->dynimpname && HEADTYPE == 10)
+			continue;
 		if(s->type != SDATA) {
 			if(s->type == SDATA1)
 				s->type = SDATA;
@@ -122,6 +127,19 @@
 		datsize += t;
 	}
 
+	/* allocate block for windows dynimport vars */
+	if(HEADTYPE == 10) {
+		iatbegin = datsize;
+		for(i=0; i<NHASH; i++)
+		for(s = hash[i]; s != S; s = s->link) {
+			if(!s->dynimpname)
+				continue;
+			t = s->size;
+			s->value = datsize;
+			datsize += t;
+		}
+	}
+
 	if(debug['j']) {
 		/*
 		 * pad data with bss that fits up to next

Here, we don't want to change the way other platforms generate their files. As
to PE generation, all we do is reserving a block of (number of dll functions) *
4bytes, which will start at iatbegin.

Next, I suggest, you change ld\pe.c to create contents of that block in memory
(just create array of 32bit ints) by walking dll functions list, and then write
its contents in one go (starting at iatbegin). You could use the same array to
produce both OriginalFirstThunk and FirstThunk data - these should be the same.


Alex

[vcc, 2010-10-12 15:49:18]

Make code a bit simple. Thanks for review, please take another look.

On 2010/10/05 02:00:12, brainman wrote:
> ...
> +		if(s->dynimpname && HEADTYPE == 10)
> +			continue;

done.

> ...
> +	/* allocate block for windows dynimport vars */
> +	if(HEADTYPE == 10) {
> +		iatbegin = datsize;
> +		for(i=0; i<NHASH; i++)
> +		for(s = hash[i]; s != S; s = s->link) {
> +			if(!s->dynimpname)
> +				continue;
> +			t = s->size;
> +			s->value = datsize;
> +			datsize += t;
> +		}
> +	}
> +

Since Thunks arrange by dlls, so need more work here.

>  	if(debug['j']) {
>  		/*
>  		 * pad data with bss that fits up to next
> 
> Here, we don't want to change the way other platforms generate their files. As
> to PE generation, all we do is reserving a block of (number of dll functions)
*
> 4bytes, which will start at iatbegin.
> 
> Next, I suggest, you change ld\pe.c to create contents of that block in memory
> (just create array of 32bit ints) by walking dll functions list, and then
write
> its contents in one go (starting at iatbegin). You could use the same array to
> produce both OriginalFirstThunk and FirstThunk data - these should be the
same.
> 
> 
> Alex

[brainman, 2010-10-12 23:59:21]

On 2010/10/12 15:49:18, vcc wrote:
> > ...
> > +	/* allocate block for windows dynimport vars */
> > +	if(HEADTYPE == 10) {
> > +		iatbegin = datsize;
> > +		for(i=0; i<NHASH; i++)
> > +		for(s = hash[i]; s != S; s = s->link) {
> > +			if(!s->dynimpname)
> > +				continue;
> > +			t = s->size;
> > +			s->value = datsize;
> > +			datsize += t;
> > +		}
> > +	}
> > +
> 
> Since Thunks arrange by dlls, so need more work here.
> 

No, you should not need anything else. Just think about it, you have everything
you need. You have bunch of symbols s (for(s = hash[i]; s != S; s = s->link)
if(s->dynimpname){...}). You should create XXX array of uint32 (Thunks or
whatever they are called), and store Thunk for each symbol s into
XXX[(s->value-iatbegin)/sizeof(uint32)]. Once done, just write XXX array into
executable starting from offset iatbegin.

Alex

[vcc, 2010-10-16 06:35:30]

On 2010/10/12 23:59:21, brainman wrote:
> 
> No, you should not need anything else. Just think about it, you have
everything
> you need. You have bunch of symbols s (for(s = hash[i]; s != S; s = s->link)
> if(s->dynimpname){...}). You should create XXX array of uint32 (Thunks or
> whatever they are called), and store Thunk for each symbol s into
> XXX[(s->value-iatbegin)/sizeof(uint32)]. Once done, just write XXX array into
> executable starting from offset iatbegin.

Just allocate block is not enough，need exact as OriginalFirstThunk,

[
DLL1[Func1,...],0,
DLL2[Func1,...],0,
...
]

[vcc, 2010-10-16 06:39:02]

update code, please take another look.

Thanks for review, please consider submit this CL, so I can move forward to
upload CL for 6g&6l and runtime X64 port.

Thanks!

[mattn, 2010-10-29 06:31:11]

Hmm, It seems that patch above is bits old?
I got an error while applying patch.

# hg sync
# hg clpatch 2166041
edit src/cmd/ld/data.c: patch did not apply cleanly
hgpatch failed


On 2010/10/16 06:39:02, vcc wrote:
> update code, please take another look.
> 
> Thanks for review, please consider submit this CL, so I can move forward to
> upload CL for 6g&6l and runtime X64 port.
> 
> Thanks!

[vcc, 2010-10-29 09:01:39]

2010/10/29 <mattn.jp@gmail.com>

> Hmm, It seems that patch above is bits old?
> I got an error while applying patch.
>
> # hg sync
> # hg clpatch 2166041
> edit src/cmd/ld/data.c: patch did not apply cleanly
> hgpatch failed
>

hg uploaded, please try again.

https://codereview.appspot.com/2166041/

[rsc1, 2010-11-01 21:50:44]

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

http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/data.c#newcode691
src/cmd/ld/data.c:691: /* allocate FirstThunk block for windows dynimport vars
*/
This shouldn't be necessary.  The correct test would be
    if(thechar == '8' && HEADTYPE == 10)
but even so it should not be necessary.  Look at
how SMACHO and SSUB get used for the Mach-O
dynamic imports.  Something like that should work
here too, and then the special purpose code here
can go away.

http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c#newcode135
src/cmd/ld/pe.c:135: peInitDynimport() {
brace on next line

http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c#newcode185
src/cmd/ld/pe.c:185: peSetIAT(int datsize) {
brace on next line

[vcc, 2010-11-02 15:34:21]

2010/11/2 <rsc@google.com>

>
> http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/data.c
> File src/cmd/ld/data.c (right):
>
>
> http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/data.c#newcode691
> src/cmd/ld/data.c:691: /* allocate FirstThunk block for windows
> dynimport vars */
> This shouldn't be necessary.  The correct test would be
>    if(thechar == '8' && HEADTYPE == 10)
> but even so it should not be necessary.  Look at
> how SMACHO and SSUB get used for the Mach-O
> dynamic imports.  Something like that should work
> here too, and then the special purpose code here
> can go away.
>
>
I don't know Mach-O, after search SMACHO in source code, I see big
difference  between Mach-O and PE, i have no idea how to reuse code for
Mach-O.

http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c
> File src/cmd/ld/pe.c (right):
>
> http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c#newcode135
> src/cmd/ld/pe.c:135: peInitDynimport() {
> brace on next line
>
> http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c#newcode185
> src/cmd/ld/pe.c:185: peSetIAT(int datsize) {
> brace on next line
>
> http://codereview.appspot.com/2166041/
>

[rsc, 2010-11-02 16:07:21]

The address assignment code where you added the special
case for Windows should be general enough to handle the
Windows dynamic pointers without modification.  Instead of
adding special case code, you should be able to define a single
symbol with type SWINDOWS and then make the other symbols
sub-symbols of it.  The address code will pick an address for
the SWINDOWS symbol and then compute the addressed of all
the subsymbols by adding their value fields to the base address
chosen for the outer symbol.

The Mach-O binary has the same kind of array of dynamic pointers,
and there the outer symbol has type SMACHO and the inner symbols
have type SMACHO | SSUB.  The outer symbol is named __nl_symbol_ptr.
I believe the Windows code can do basically the same thing.

Russ

[vcc, 2010-11-04 18:02:12]

Move dynamic import address assignment code into pe.c, don't touch dodata
anymore, please take another look.

On 2010/11/02 16:07:21, rsc wrote:
> The address assignment code where you added the special
> case for Windows should be general enough to handle the
> Windows dynamic pointers without modification.  Instead of
> adding special case code, you should be able to define a single
> symbol with type SWINDOWS and then make the other symbols
> sub-symbols of it.  The address code will pick an address for
> the SWINDOWS symbol and then compute the addressed of all
> the subsymbols by adding their value fields to the base address
> chosen for the outer symbol.
> 
> The Mach-O binary has the same kind of array of dynamic pointers,
> and there the outer symbol has type SMACHO and the inner symbols
> have type SMACHO | SSUB.  The outer symbol is named __nl_symbol_ptr.
> I believe the Windows code can do basically the same thing.
> 
> Russ

[rsc1, 2010-11-04 18:10:13]

definitely better, thanks.

https://codereview.appspot.com/2166041/diff/80001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

https://codereview.appspot.com/2166041/diff/80001/src/cmd/ld/pe.c#newcode151
src/cmd/ld/pe.c:151: int i;
Sym *dynamic;

dynamic = lookup(".windynamic", 0);
dynamic->reachable = 1;
dynamic->type = SWINDOWS;

https://codereview.appspot.com/2166041/diff/80001/src/cmd/ld/pe.c#newcode163
src/cmd/ld/pe.c:163: //s->type = SWINDOWS;
s->type = SWINDOWS | SSUB;
s->outer = dynamic;
s->sub = dynamic->sub;
dynamic->sub = s;
s->value = dynamic->size;
dynamic->size += 4;

https://codereview.appspot.com/2166041/diff/80001/src/cmd/ld/pe.c#newcode300
src/cmd/ld/pe.c:300: for(d = dr; d != nil; d = d->next) {
I'd prefer not to have address selection code outside of data.c.
With the changes above data.c will do the right thing for you.

[brainman, 2010-11-04 22:33:26]

On 2010/11/04 18:02:12, vcc wrote:
> Move dynamic import address assignment code into pe.c, don't touch dodata
> anymore, please take another look.
> 

I don't think it is what Russ meant to do. If you willing to wait a couple of
days, I'll send you my interpretation.

Alex

[vcc, 2010-11-05 09:19:24]

All done, thanks Russ, please take another look.

http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c#newcode135
src/cmd/ld/pe.c:135: peInitDynimport() {
On 2010/11/01 21:50:44, rsc1 wrote:
> brace on next line

Done.

http://codereview.appspot.com/2166041/diff/70001/src/cmd/ld/pe.c#newcode185
src/cmd/ld/pe.c:185: peSetIAT(int datsize) {
On 2010/11/01 21:50:44, rsc1 wrote:
> brace on next line

Done.

http://codereview.appspot.com/2166041/diff/80001/src/cmd/ld/pe.c#newcode151
src/cmd/ld/pe.c:151: for(d = dr; d != nil; d = d->next) {
On 2010/11/04 18:10:13, rsc1 wrote:
> Sym *dynamic;
> 
> dynamic = lookup(".windynamic", 0);
> dynamic->reachable = 1;
> dynamic->type = SWINDOWS;

Done.

http://codereview.appspot.com/2166041/diff/80001/src/cmd/ld/pe.c#newcode163
src/cmd/ld/pe.c:163: d = mal(sizeof *d);
On 2010/11/04 18:10:13, rsc1 wrote:
> s->type = SWINDOWS | SSUB;
> s->outer = dynamic;
> s->sub = dynamic->sub;
> dynamic->sub = s;
> s->value = dynamic->size;
> dynamic->size += 4;

Done.

http://codereview.appspot.com/2166041/diff/80001/src/cmd/ld/pe.c#newcode300
src/cmd/ld/pe.c:300: fh.Characteristics = IMAGE_FILE_RELOCS_STRIPPED|
On 2010/11/04 18:10:13, rsc1 wrote:
> I'd prefer not to have address selection code outside of data.c.
> With the changes above data.c will do the right thing for you.

Done.

[rsc1, 2010-11-05 15:58:47]

looks good; just a few things below

http://codereview.appspot.com/2166041/diff/94001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/94001/src/cmd/ld/pe.c#newcode145
src/cmd/ld/pe.c:145: initdynimport()
s/()/(void)/

http://codereview.appspot.com/2166041/diff/94001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/2166041/diff/94001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:36: void *CreateEvent;
If these are no longer static, they need to say runtime· to avoid conflicts.

http://codereview.appspot.com/2166041/diff/94001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:51: void *GetCommandLine;
same

[vcc, 2010-11-05 16:35:27]

Thanks for review, please take another look.

http://codereview.appspot.com/2166041/diff/94001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/94001/src/cmd/ld/pe.c#newcode145
src/cmd/ld/pe.c:145: initdynimport()
On 2010/11/05 15:58:47, rsc1 wrote:
> s/()/(void)/

Done.

http://codereview.appspot.com/2166041/diff/94001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/2166041/diff/94001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:36: void *CreateEvent;
On 2010/11/05 15:58:47, rsc1 wrote:
> If these are no longer static, they need to say runtime· to avoid conflicts.

Done.

http://codereview.appspot.com/2166041/diff/94001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:51: void *GetCommandLine;
On 2010/11/05 15:58:47, rsc1 wrote:
> same

Done.

[brainman, 2010-11-09 04:00:05]

You are getting there.

Please check files you send for review. I suspect they all have wrong line
endings (CR+LF), not (LF). It is not a big deal, but it is hard to see your
changes, as codereview shows whole file is changed, which is not true.

Thank you.

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

http://codereview.appspot.com/2166041/diff/86002/src/cmd/8l/obj.c#newcode324
src/cmd/8l/obj.c:324: dope();
Do not like the name - "dope" what? Perhaps, "initpedynimport", then you do not
need to have dope call initdynimport?

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode220
src/cmd/ld/pe.c:220: isect = addpesection(".idata", nsize, nsize, 0);
You're probably good with arithmetic, but, I'm not <g>. If you move
addpesection() to after you've written all you data, you won't need to calculate
nsize by hand, instead, you could just use seek(cout, 0, 2) at the end and
compare it with the one at the start.

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode230
src/cmd/ld/pe.c:230: iat_off = dynamic->value - PEBASE; // FirstThunk allocated
in .data
You should be able to set another "directory entry" in pe header now:

oh.DataDirectory[IMAGE_DIRECTORY_ENTRY_IAT].VirtualAddress = dynamic->value -
PEBASE;
oh.DataDirectory[IMAGE_DIRECTORY_ENTRY_IAT].Size = dynamic->size;

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode264
src/cmd/ld/pe.c:264: strput(d->name);
You must pad this string: from http://kishorekumar.net/pecoff_v8.1.htm - "A
trailing zero-pad byte that appears after the trailing null byte, if necessary,
to align the next entry on an even boundary."

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode270
src/cmd/ld/pe.c:270: strput(m->s->dynimpname);
Same as above.

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode277
src/cmd/ld/pe.c:277: // put FirstThunk
This code is exact copy of the one for OriginalFirstThunk with exception of
seek() at the start. I would emphasize that by putting it in a separate
function, and calling it from appropriate place.

http://codereview.appspot.com/2166041/diff/86002/src/pkg/runtime/windows/mem.c
File src/pkg/runtime/windows/mem.c (right):

http://codereview.appspot.com/2166041/diff/86002/src/pkg/runtime/windows/mem....
src/pkg/runtime/windows/mem.c:20: void *runtime·VirtualAlloc;
static void *VirtualAlloc;

Also, please, check all other similar variables. There are a lot of them that
should be marked as "static".

[vcc, 2010-11-15 12:53:55]

Please take another look.

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

http://codereview.appspot.com/2166041/diff/86002/src/cmd/8l/obj.c#newcode324
src/cmd/8l/obj.c:324: dope();
On 2010/11/09 04:00:06, brainman wrote:
> Do not like the name - "dope" what? Perhaps, "initpedynimport", then you do
not
> need to have dope call initdynimport?

Just like doelf, domacho, maybe will do initdynexport in dope in the future.

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode220
src/cmd/ld/pe.c:220: isect = addpesection(".idata", nsize, nsize, 0);
On 2010/11/09 04:00:06, brainman wrote:
> You're probably good with arithmetic, but, I'm not <g>. If you move
> addpesection() to after you've written all you data, you won't need to
calculate
> nsize by hand, instead, you could just use seek(cout, 0, 2) at the end and
> compare it with the one at the start.

But we need VirtualAddress, so do addpesection first.

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode230
src/cmd/ld/pe.c:230: iat_off = dynamic->value - PEBASE; // FirstThunk allocated
in .data
On 2010/11/09 04:00:06, brainman wrote:
> You should be able to set another "directory entry" in pe header now:
> 
> oh.DataDirectory[IMAGE_DIRECTORY_ENTRY_IAT].VirtualAddress = dynamic->value -
> PEBASE;
> oh.DataDirectory[IMAGE_DIRECTORY_ENTRY_IAT].Size = dynamic->size;

Done.

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode264
src/cmd/ld/pe.c:264: strput(d->name);
On 2010/11/09 04:00:06, brainman wrote:
> You must pad this string: from http://kishorekumar.net/pecoff_v8.1.htm - "A
> trailing zero-pad byte that appears after the trailing null byte, if
necessary,
> to align the next entry on an even boundary."

These are dll names, don't need trailing zero-pad byte.

http://codereview.appspot.com/2166041/diff/86002/src/cmd/ld/pe.c#newcode270
src/cmd/ld/pe.c:270: strput(m->s->dynimpname);
On 2010/11/09 04:00:06, brainman wrote:
> Same as above.

This is a option.

[brainman, 2010-11-16 03:51:50]

You've ignored all (but one) my comments! But never mind that, I'll try to do
all changes myself <g>.

Just 2 more comments. Thank you.

http://codereview.appspot.com/2166041/diff/109001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/109001/src/cmd/ld/pe.c#newcode322
src/cmd/ld/pe.c:322: d = addpesection(".data", segdata.len, segdata.len,
&segdata);
Why did you change this line?

http://codereview.appspot.com/2166041/diff/109001/src/pkg/runtime/windows/mem.c
File src/pkg/runtime/windows/mem.c (right):

http://codereview.appspot.com/2166041/diff/109001/src/pkg/runtime/windows/mem...
src/pkg/runtime/windows/mem.c:45: }
Do not need to change this line.

[vcc, 2010-11-16 05:42:05]

Please take another look. I also convert pe.c to LF format, every time after do
merge that file format will change to CRLF format. I use p4merge on windows.

Thanks for review.

http://codereview.appspot.com/2166041/diff/86002/src/pkg/runtime/windows/mem.c
File src/pkg/runtime/windows/mem.c (right):

http://codereview.appspot.com/2166041/diff/86002/src/pkg/runtime/windows/mem....
src/pkg/runtime/windows/mem.c:20: void *runtime·VirtualAlloc;
On 2010/11/09 04:00:06, brainman wrote:
> static void *VirtualAlloc;
> 
> Also, please, check all other similar variables. There are a lot of them that
> should be marked as "static".

Can't be static, static has version, dynimport can't find it by name.

http://codereview.appspot.com/2166041/diff/109001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/109001/src/cmd/ld/pe.c#newcode322
src/cmd/ld/pe.c:322: d = addpesection(".data", segdata.len, segdata.len,
&segdata);
On 2010/11/16 03:51:50, brainman wrote:
> Why did you change this line?

Yes, don't need change this line right now. early patch direct append firstthunk
in .data, segdata.len will large than filelen, so make this change. 
I think change to segdata.len should better for understand, add .text section
also use segtext.len, that don't be surprise why .text use segtext.len but .data
use segdata.filelen.

http://codereview.appspot.com/2166041/diff/109001/src/pkg/runtime/windows/mem.c
File src/pkg/runtime/windows/mem.c (right):

http://codereview.appspot.com/2166041/diff/109001/src/pkg/runtime/windows/mem...
src/pkg/runtime/windows/mem.c:45: }
On 2010/11/16 03:51:50, brainman wrote:
> Do not need to change this line.

Yes, I don't know why hg mark this line is modified. I got it, need add empty
line at the last of file.

[brainman, 2010-11-16 05:58:10]

http://codereview.appspot.com/2166041/diff/109001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2166041/diff/109001/src/cmd/ld/pe.c#newcode322
src/cmd/ld/pe.c:322: d = addpesection(".data", segdata.len, segdata.len,
&segdata);
On 2010/11/16 05:42:06, vcc wrote:
> I think change to segdata.len should better for understand, add .text section
> also use segtext.len, that don't be surprise why .text use segtext.len but
.data
> use segdata.filelen.

There is a difference between .text and .data sections here.

.data section contains "initialised data" (it's length is segdata.filelen) and
"non-initalised data / bss" (it's length is segdata.len - segdata.filelen).
There is no point writing "bss" data to the disk, windows loader will create
space in memory filled with 0 for it automatically. In fact, 8l doesn't write
any data to disk for 'bss' part - if you check, .data section is of
segdata.filelen length on disk. But in memory it needs to be extended to full
segdata.len to fit both "initialised" and "non-initialised" part.

Please, revert this line to the original.

[vcc, 2010-11-16 12:27:29]

Done, please take another look.

On 2010/11/16 05:58:10, brainman wrote:
> http://codereview.appspot.com/2166041/diff/109001/src/cmd/ld/pe.c
> File src/cmd/ld/pe.c (right):
> 
> http://codereview.appspot.com/2166041/diff/109001/src/cmd/ld/pe.c#newcode322
> src/cmd/ld/pe.c:322: d = addpesection(".data", segdata.len, segdata.len,
> &segdata);
> On 2010/11/16 05:42:06, vcc wrote:
> > I think change to segdata.len should better for understand, add .text
section
> > also use segtext.len, that don't be surprise why .text use segtext.len but
> .data
> > use segdata.filelen.
> 
> There is a difference between .text and .data sections here.
> 
> .data section contains "initialised data" (it's length is segdata.filelen) and
> "non-initalised data / bss" (it's length is segdata.len - segdata.filelen).
> There is no point writing "bss" data to the disk, windows loader will create
> space in memory filled with 0 for it automatically. In fact, 8l doesn't write
> any data to disk for 'bss' part - if you check, .data section is of
> segdata.filelen length on disk. But in memory it needs to be extended to full
> segdata.len to fit both "initialised" and "non-initialised" part.
> 
> Please, revert this line to the original.

[brainman, 2010-11-16 23:25:28]

LGTM. Leaving for Russ to OK.

Thank you. It was a big job!

[rsc, 2010-12-07 18:43:11]

LGTM

Thanks.  I bet my upcoming cgo changes will
break this but after that there should be calm.

[rsc, 2010-12-07 20:28:36]

*** Submitted as f5ea1dcf89ae ***

8l : add dynimport to import table in Windows PE, initial make cgo dll work.

R=rsc, brainman, Joe Poirier, mattn
CC=golang-dev
http://codereview.appspot.com/2166041

Committer: Russ Cox <rsc@golang.org>