code review 2731041: 8l: pe generation fixes

8l: pe generation fixes

Restore ability to have different file and
section alignment in generated pe file.

Stop generating .bss pe section, it is
part of .data now.

Some code refactoring.

[brainman, 2010-10-25 23:49:04]

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

I'd like you to review this change.

[rsc1, 2010-10-25 23:52:52]

thanks for fixing this yet again

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

http://codereview.appspot.com/2731041/diff/2001/src/cmd/ld/data.c#newcode740
src/cmd/ld/data.c:740: if(HEADTYPE == 10)
if(thechar == '8' && HEADTYPE == 10)  // Windows PE

(Because this code is used for 6l and 5l too.)

Also, please leave the current assignment alone
and then just override it, without an else:

segdata.fileoff = va - segtext.vaddr + segtext.fileoff;
if(thechar == '8' && HEADTYPE == 10)
    segdata.fileoff = something else;

so that it's easier to see the default behavior.
right now when i get to line 739 i think
"well that's obviously wrong".

[brainman, 2010-10-26 00:15:37]

Thanks for review.

I get:

--- cd ../test                                            
fail: ./recover2.go                                       
5,7c5,18                                                  
< panic: runtime error: comparing uncomparable type []int 
<                                                         
< panic PC=xxx                                            
---                                                       
> "#runtime error: comparing uncomparable type []intruntim
/runtime/proc.c:1028                                      
>       runtime.panic(0x805529c, 0xb7e520f8)              
> ifaceeq1+0xba /root/hg/go/src/pkg/runtime/iface.c:557   
>       ifaceeq1(0xb7e64000, 0xb7e64000, 0x80556bc, 0xb7e5
> efaceeq+0x59 /root/hg/go/src/pkg/runtime/iface.c:582    
>       efaceeq(0x80556bc, 0xb7e64000, 0x80556bc, 0xb7e640
> runtime.efaceeq+0x41 /root/hg/go/src/pkg/runtime/iface.c
>       runtime.efaceeq(0x80556bc, 0xb7e64000, 0x80556bc, 
> main.main+0x62 /root/hg/go/test/./cmp2.go:14            
>       main.main()                                       
> mainstart+0xf /root/hg/go/src/pkg/runtime/386/asm.s:84  
>       mainstart()                                       
> goexit /root/hg/go/src/pkg/runtime/proc.c:146           
>       goexit()                                          
10,12c21,34                                               
< panic: runtime error: comparing uncomparable type []int 
<                                                         
< panic PC=xxx                                            
--- 
...
1 known bugs; 0 unexpected bugs; test output differs
FAILED

while running cd %GOROOT/src ; ./all.bash.

I can't see my change causing that. Did it?

Alex

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

http://codereview.appspot.com/2731041/diff/2001/src/cmd/ld/data.c#newcode740
src/cmd/ld/data.c:740: if(HEADTYPE == 10)
On 2010/10/25 23:52:52, rsc1 wrote:
> if(thechar == '8' && HEADTYPE == 10)  // Windows PE
> 
> (Because this code is used for 6l and 5l too.)
> 
> Also, please leave the current assignment alone
> and then just override it, without an else:
> 
> segdata.fileoff = va - segtext.vaddr + segtext.fileoff;
> if(thechar == '8' && HEADTYPE == 10)
>     segdata.fileoff = something else;
> 
> so that it's easier to see the default behavior.
> right now when i get to line 739 i think
> "well that's obviously wrong".

Done.

[brainman, 2010-10-26 00:15:51]

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

Please take another look.

[brainman, 2010-10-26 00:26:36]

> ...
> 1 known bugs; 0 unexpected bugs; test output differs
> FAILED

Forget to add my tip is:

changeset:   6613:b4a830419384                                          
tag:         tip                                                        
user:        Andrew Gerrand <adg@golang.org>                            
date:        Mon Oct 25 16:38:48 2010 +1100                             
summary:     build: only print "You need to add foo to PATH" when needed

[rsc, 2010-10-26 00:41:01]

>> "#runtime error: comparing uncomparable type []intruntim
>
> /runtime/proc.c:1028
>>
>>       runtime.panic(0x805529c, 0xb7e520f8)

does it really look like this or did the output get mangled in mail?
can you paste it on a paste site instead?

[brainman, 2010-10-26 00:54:40]

On 2010/10/26 00:41:01, rsc wrote:
> does it really look like this or did the output get mangled in mail?
> can you paste it on a paste site instead?

I've updated to:

changeset:   6621:c1ef5b0ff968                                    
tag:         tip                                                  
user:        Rob Pike <r@golang.org>                              
date:        Mon Oct 25 17:03:25 2010 -0700                       
summary:     gotest: generate correct gofmt-formatted _testmain.go

but $GOROOT/src/all.bash still fails:

http://pastebin.com/TCYQMb2U

My terminal program doesn't support line mode copy, only block mode. Sorry.

[rsc, 2010-10-26 01:00:11]

I think this is your change.
Specifically I think that the file offset where the
PE header says the data begins and the actual
file offset where the data begins are different,
causing references to globals like strings and
type reflection data to go wrong.  It could be the
read-only data inside the text segment that is
being mis-located, not the read-write data in the
data segment.

Russ

[brainman, 2010-10-26 01:33:05]

> I think this is your change.

You're correct - once I remove my changes, everything builds. What I don't
understand is, since I'm running all.bash on linux (GOOS=linux GOARCH=386), I
don't expect to see any pe related issues. I look at all common files I've
changed, and I can't see how I could affect non-windows build.

[rsc1, 2010-10-26 03:26:16]

here it is (at least one)

http://codereview.appspot.com/2731041/diff/9001/src/cmd/8l/asm.c
File src/cmd/8l/asm.c (right):

http://codereview.appspot.com/2731041/diff/9001/src/cmd/8l/asm.c#newcode401
src/cmd/8l/asm.c:401: symo = rnd(HEADR+segtext.filelen,
PEFILEALIGN)+segdata.filelen;
This code is case 10 but also Elfsym

[brainman, 2010-10-26 03:45:48]

http://codereview.appspot.com/2731041/diff/9001/src/cmd/8l/asm.c
File src/cmd/8l/asm.c (right):

http://codereview.appspot.com/2731041/diff/9001/src/cmd/8l/asm.c#newcode401
src/cmd/8l/asm.c:401: symo = rnd(HEADR+segtext.filelen,
PEFILEALIGN)+segdata.filelen;
On 2010/10/26 03:26:16, rsc1 wrote:
> This code is case 10 but also Elfsym

Yep, I've found it too. I will fix Elfsym case, but not sure what to do for 10.
What are we looking for here? Start of ".rodata" section inside segtext segment?
Never mind, I'll work it out. Important thing, I still don't believe in magic
<g>.

Alex

[brainman, 2010-10-26 04:20:10]

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

Please take another look.

[brainman, 2010-10-30 03:37:12]

On 2010/10/26 04:20:10, brainman wrote:
> Hello rsc, vcc (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Ping.

[vcc, 2010-10-30 12:33:27]

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

http://codereview.appspot.com/2731041/diff/18001/src/cmd/ld/pe.c#newcode70
src/cmd/ld/pe.c:70: nextsectoff = rndsect(nextsectoff+sectsize);
rnd(nextsectoff+sectsize, PESECTALIGN) should ok.

http://codereview.appspot.com/2731041/diff/18001/src/cmd/ld/pe.h
File src/cmd/ld/pe.h (right):

http://codereview.appspot.com/2731041/diff/18001/src/cmd/ld/pe.h#newcode83
src/cmd/ld/pe.h:83: extern	int32	PESECTHEADR;
PESECTHEADR should be 0x1000, can use PESECTALIGN instead.

http://codereview.appspot.com/2731041/diff/18001/src/cmd/ld/pe.h#newcode84
src/cmd/ld/pe.h:84: extern	int32	PEFILEHEADR;
PEFILEHEADR is 1024 right now, #define PEFILEHEADR 0x400 should be ok.

[brainman, 2010-11-01 05:15:46]

Thank you for review.

http://codereview.appspot.com/2731041/diff/18001/src/cmd/8l/asm.c
File src/cmd/8l/asm.c (right):

http://codereview.appspot.com/2731041/diff/18001/src/cmd/8l/asm.c#newcode405
src/cmd/8l/asm.c:405: symo = rnd(HEADR+segtext.filelen,
PEFILEALIGN)+segdata.filelen;
Wei, this code is not currently used, but, I believe, you're using debug
information in pe. Could you, please check that you have no trouble finding all
your debug info after this CL is submitted.

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

http://codereview.appspot.com/2731041/diff/18001/src/cmd/ld/pe.c#newcode70
src/cmd/ld/pe.c:70: nextsectoff = rndsect(nextsectoff+sectsize);
On 2010/10/30 12:33:27, vcc wrote:
> rnd(nextsectoff+sectsize, PESECTALIGN) should ok.

Done.

http://codereview.appspot.com/2731041/diff/18001/src/cmd/ld/pe.h
File src/cmd/ld/pe.h (right):

http://codereview.appspot.com/2731041/diff/18001/src/cmd/ld/pe.h#newcode83
src/cmd/ld/pe.h:83: extern	int32	PESECTHEADR;
On 2010/10/30 12:33:27, vcc wrote:
> PESECTHEADR should be 0x1000, can use PESECTALIGN instead.

One of the goals of this CL is to allow us to have any (within PE spec) sizes
for FileAlignment and SectionAlignment. Once these are defined by PESECTALIGN
and PEFILEALIGN defines, PESECTHEADR and PEFILEHEADR could be calculated, as is
done in pe.c/peinit(). We just need to make sure that our PE header fit inside
PEFILEHEADR, that PEFILEHEADR <= PESECTHEADR, and PEFILEHEADR is PESECTALIGN
aligned, and PESECTHEADR is PESECTALIGN aligned.

http://codereview.appspot.com/2731041/diff/18001/src/cmd/ld/pe.h#newcode84
src/cmd/ld/pe.h:84: extern	int32	PEFILEHEADR;
On 2010/10/30 12:33:27, vcc wrote:
> PEFILEHEADR is 1024 right now, #define PEFILEHEADR 0x400 should be ok.

Same as above.

[brainman, 2010-11-01 05:15:47]

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

Please take another look.

[rsc1, 2010-11-01 18:31:36]

LGTM

[brainman, 2010-11-01 23:57:04]

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

8l: pe generation fixes

Restore ability to have different file and
section alignment in generated pe file.

Stop generating .bss pe section, it is
part of .data now.

Some code refactoring.

R=rsc, vcc
CC=golang-dev
http://codereview.appspot.com/2731041

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