code review 4182061: 6l: pe fixes

6l: pe fixes

.data length need set to segdata.len for make gdb works.

[vcc, 2011-02-16 16:24:57]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[rsc, 2011-02-16 16:36:29]

> -                       && p->from.offset != 0x58) {
> +                       && p->from.offset != 0x58 && p->from.offset != 0x30)

This is not scalable.
If the goal is to distinguish 0(x) and 4(x) and maybe 8(x) from the
others, let's use

p->from.offset > 8.

[rsc, 2011-02-16 18:11:40]

or maybe p->from.offset <= 8  :-)

[brainman, 2011-02-16 22:16:33]

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

http://codereview.appspot.com/4182061/diff/2001/src/cmd/ld/pe.c#newcode484
src/cmd/ld/pe.c:484: d = addpesection(".data", segdata.len, pe64 ? segdata.len :
segdata.filelen, &segdata);
This is 3rd time you're making this change. Please, explain why you need to have
this extra blob of data on the disk filled with 0.

[vcc, 2011-02-17 03:24:17]

2011/2/17 <alex.brainman@gmail.com>

>
> http://codereview.appspot.com/4182061/diff/2001/src/cmd/ld/pe.c
> File src/cmd/ld/pe.c (right):
>
> http://codereview.appspot.com/4182061/diff/2001/src/cmd/ld/pe.c#newcode484
> src/cmd/ld/pe.c:484: d = addpesection(".data", segdata.len, pe64 ?
>
> segdata.len : segdata.filelen, &segdata);
> This is 3rd time you're making this change. Please, explain why you need
> to have this extra blob of data on the disk filled with 0.


for X64 gdb need, otherwise gdb can't get .debug section data right.


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

[brainman, 2011-02-17 04:52:59]

On 2011/02/17 03:24:17, vcc wrote:
> 
> for X64 gdb need, otherwise gdb can't get .debug section data right.
> 

Please, convince me.

Alex

[vcc, 2011-02-17 14:30:08]

2011/2/17 <alex.brainman@gmail.com>

> On 2011/02/17 03:24:17, vcc wrote:
>
>  for X64 gdb need, otherwise gdb can't get .debug section data right.
>>
>
>
> Please, convince me.
>

If use segdata.filelen, gdb can't load symbols:
$ gdb h
GNU gdb (GDB) 7.1
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html
>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-w64-mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from g:\opensource\go-w64\win64/h.exe...Dwarf Error: wrong
version in compilation unit header (is 5014, should be 2) [in module
g:\opensource\go-w64\win64/h.exe]

change to just use segdata.len when generate debug information should be ok:
    if(!debug['s'] && pe64)
        d = addpesection(".data", segdata.len, segdata.len, &segdata);
    else
        d = addpesection(".data", segdata.len, segdata.filelen, &segdata);


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

[vcc, 2011-02-17 14:33:17]

2011/2/17 <rsc@golang.org>

> or maybe p->from.offset <= 8  :-)


Done.


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

[vcc, 2011-02-17 14:33:40]

PTAL

[rsc1, 2011-02-17 14:57:40]

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

http://codereview.appspot.com/4182061/diff/11001/src/cmd/ld/pe.c#newcode485
src/cmd/ld/pe.c:485: d = addpesection(".data", segdata.len, segdata.len,
&segdata);
I believe you that it works, but this can't possibly be the correct fix.
Try compiling this Go program:

package main
var x [1e9]byte
func main() {
    println(x[0])
}

The 6.out is a few hundred kilobytes on my system.  With your
change it will be a gigabyte.

I believe that gdb is getting confused, but I wonder if it's 
more a matter of proper section alignment.  Maybe the filelen
needs to be chosen differently, to align on a 512 or 4096 or
something like that byte boundary?

Please take this change out of the CL for now.  Then the CL
looks good and can be submitted.  A future CL can address
the section problem once we understand what the correct fix is.

Maybe it would help to look at the source code in gdb that is
reading the PE files to see what it expects.

[vcc, 2011-02-17 15:32:20]

PTAL

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

http://codereview.appspot.com/4182061/diff/11001/src/cmd/ld/pe.c#newcode485
src/cmd/ld/pe.c:485: d = addpesection(".data", segdata.len, segdata.len,
&segdata);
On 2011/02/17 14:57:41, rsc1 wrote:
> I believe you that it works, but this can't possibly be the correct fix.
> Try compiling this Go program:
> 
> package main
> var x [1e9]byte
> func main() {
>     println(x[0])
> }
> 
> The 6.out is a few hundred kilobytes on my system.  With your
> change it will be a gigabyte.
> 
> I believe that gdb is getting confused, but I wonder if it's 
> more a matter of proper section alignment.  Maybe the filelen
> needs to be chosen differently, to align on a 512 or 4096 or
> something like that byte boundary?
> 
> Please take this change out of the CL for now.  Then the CL
> looks good and can be submitted.  A future CL can address
> the section problem once we understand what the correct fix is.
> 
> Maybe it would help to look at the source code in gdb that is
> reading the PE files to see what it expects.
> 

Thanks Russ for point out this, done.

[rsc1, 2011-02-17 16:01:08]

LGTM

Leaving for Alex.

[brainman, 2011-02-17 23:58:38]

On 2011/02/17 15:32:20, vcc wrote:
> PTAL
> 

LGTM

[brainman, 2011-02-17 23:58:55]

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

6l: pe fixes

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

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

[brainman, 2011-02-18 00:02:34]

On 2011/02/17 14:57:40, rsc1 wrote:
> 
> Maybe it would help to look at the source code in gdb that is
> reading the PE files to see what it expects.

Tried it. I'll try again one day, when I'm brave enough!

Alex