code review 4516055: 8l: emit resources (.rsrc) in Windows PE.

8l: emit resources (.rsrc) in Windows PE.

[vcc, 2011-05-15 06:59:22]

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

I'd like you to review this change.

[rsc, 2011-05-16 20:48:26]

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

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/ldpe.c#newcode226
src/cmd/ld/ldpe.c:226: setpersrc(sect->sh.SizeOfRawData, sect->base, sect->sym);
Since sect->sym->p == sect->base and sect->sym->size == sect->sh.SizeOfRawData,
do we need three parameters?

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

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/pe.c#newcode466
src/cmd/ld/pe.c:466: setpersrc(int size, void *data, Sym *sym)
It looks like this means only one .rsrc section can be loaded,
since the second one will overwrite the first one.  Should this say

if(sym != nil)
    diag("too many .rsrc sections");

?

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/pe.c#newcode487
src/cmd/ld/pe.c:487: long *l = (long*)&((char*)rsrcdata)[rsrcsym->r[i].off];
This assumes that on the machine running 8l,
long is 32 bits little endian (to match the 386).
The linker can run on other systems, so this is not
a valid assumption.

A better phrasing of this is

(at top of function)
    uchar *p;
    uint32 val;
    Reloc *r;

(here)
    for(r=rsrcsym->r; r<rsrcsym->r+rsrcsym->nr; r++) {
        p = (uchar*)rsrcdata + r->off;
        val = h->VirtualAddress + r->add;
        // 32-bit little-endian
        p[0] = val;
        p[1] = val>>8;
        p[2] = val>>16;
        p[3] = val>>24;
    }

But more importantly, why is this code doing a relocation?
Why is it not letting relocsym do the relocation?

[brainman, 2011-05-17 00:33:19]

Welcome back and thank you.

Alex

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

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/pe.c#newcode484
src/cmd/ld/pe.c:484: IMAGE_SCN_MEM_WRITE | IMAGE_SCN_CNT_INITIALIZED_DATA;
Do not need section to be IMAGE_SCN_MEM_WRITE. Do you?

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/pe.c#newcode485
src/cmd/ld/pe.c:485: // relocation
I didn't look close enough, but I don't understand why would you need to
"relocate" some data. I'm under impression, you could just copy all data in that
section byte by byte from obj file into final exe. Isn't it?

As far as I remember all addresses inside are RVAs (Relative Virtual Addresses),
all are relative to the beginning of section.

Alex

[vcc, 2011-05-17 08:20:30]

PTAL

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

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/ldpe.c#newcode226
src/cmd/ld/ldpe.c:226: setpersrc(sect->sh.SizeOfRawData, sect->base, sect->sym);
On 2011/05/16 20:48:26, rsc wrote:
> Since sect->sym->p == sect->base and sect->sym->size ==
sect->sh.SizeOfRawData,
> do we need three parameters?

Only use sym is ok, Done.

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

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/pe.c#newcode466
src/cmd/ld/pe.c:466: setpersrc(int size, void *data, Sym *sym)
On 2011/05/16 20:48:26, rsc wrote:
> It looks like this means only one .rsrc section can be loaded,
> since the second one will overwrite the first one.  Should this say
> 
> if(sym != nil)
>     diag("too many .rsrc sections");
> 
> ?

Done.

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/pe.c#newcode484
src/cmd/ld/pe.c:484: IMAGE_SCN_MEM_WRITE | IMAGE_SCN_CNT_INITIALIZED_DATA;
On 2011/05/17 00:33:19, brainman wrote:
> Do not need section to be IMAGE_SCN_MEM_WRITE. Do you?

"Microsoft Portable Executable and Common Object File Format Specification
(revision 6.0, Feb 1999)":
.rsrc has flags
IMAGE_SCN_CNT_INITIALIZED_DATA |
IMAGE_SCN_MEM_READ |
IMAGE_SCN_MEM_WRITE

but revision 8.2, 2010, has flags
IMAGE_SCN_CNT_INITIALIZED_DATA |
IMAGE_SCN_MEM_READ

I think has IMAGE_SCN_MEM_WRITE is OK, maybe win2000 or XP expect it.

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/pe.c#newcode485
src/cmd/ld/pe.c:485: // relocation
On 2011/05/17 00:33:19, brainman wrote:
> I didn't look close enough, but I don't understand why would you need to
> "relocate" some data. I'm under impression, you could just copy all data in
that
> section byte by byte from obj file into final exe. Isn't it?
> 
> As far as I remember all addresses inside are RVAs (Relative Virtual
Addresses),
> all are relative to the beginning of section.
> 
> Alex

.rsrc RVAs in obj file is start from 0, link in exe need relocate from .rsrc
section virtual address.

http://codereview.appspot.com/4516055/diff/5001/src/cmd/ld/pe.c#newcode487
src/cmd/ld/pe.c:487: long *l = (long*)&((char*)rsrcdata)[rsrcsym->r[i].off];
On 2011/05/16 20:48:26, rsc wrote:
> This assumes that on the machine running 8l,
> long is 32 bits little endian (to match the 386).
> The linker can run on other systems, so this is not
> a valid assumption.
> 
> A better phrasing of this is
> 
> (at top of function)
>     uchar *p;
>     uint32 val;
>     Reloc *r;
> 
> (here)
>     for(r=rsrcsym->r; r<rsrcsym->r+rsrcsym->nr; r++) {
>         p = (uchar*)rsrcdata + r->off;
>         val = h->VirtualAddress + r->add;
>         // 32-bit little-endian
>         p[0] = val;
>         p[1] = val>>8;
>         p[2] = val>>16;
>         p[3] = val>>24;
>     }

Thanks Russ, Done.

> 
> But more importantly, why is this code doing a relocation?
> Why is it not letting relocsym do the relocation?
> 

.rsrc is a new section, don't in .text and .data, can't use relocsym. Also I
only see IMAGE_REL_I386_DIR32NB use in .rsrc right now, so doing relocation here
is OK.

[brainman, 2011-05-21 13:29:46]

LGTM

[rsc, 2011-05-25 11:51:35]

LGTM

[rsc, 2011-05-25 11:53:03]

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

8l: emit resources (.rsrc) in Windows PE.

R=alex.brainman, rsc
CC=golang-dev, vcc.163
http://codereview.appspot.com/4516055

Committer: Russ Cox <rsc@golang.org>