code review 5823059: cmd/ld: take section symbols' value into account for PE

cmd/ld: take section symbols' value into account for PE
    ld -r could generate multiple section symbols for the same section, 
but with different values, we have to take that into account.
    Fixes issue 3322.
    Part of issue 3261.
    For CL 5822049.

[minux1, 2012-03-15 17:40:14]

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

I'd like you to review this change to
https://code.google.com/p/go/

[minux1, 2012-03-15 17:49:19]

Tester welcome for this CL and CL 5822049 on all platforms, esp. Windows.

These two CLs will enable you to do at least these on Windows:
go build $GOROOT/misc/cgo/life/main.go
go test $GOROOT/misc/cgo/test

Any feedback welcome.

[minux1, 2012-03-15 18:08:14]

I'd also ask if why only R_X86_64_64 should take symbol value into account?
I've tried to move the added line 2 lines below, but all misc/cgo tests fail.

[minux1, 2012-03-15 18:39:21]

Wait a minute, this CL didn't fix the problem on windows/386.

[iant, 2012-03-15 20:03:48]

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

http://codereview.appspot.com/5823059/diff/3005/src/cmd/ld/ldpe.c#newcode304
src/cmd/ld/ldpe.c:304: rp->add += obj->pesym[symindex].value;
It seems to me that setting type = D_ADDR means that the symbol value will be
added into the relocation anyhow.  So I don't know why this is right.

[minux1, 2012-03-16 10:38:33]

PTAL.
On 2012/03/15 20:03:48, iant wrote:
> http://codereview.appspot.com/5823059/diff/3005/src/cmd/ld/ldpe.c
> File src/cmd/ld/ldpe.c (right):
> 
> http://codereview.appspot.com/5823059/diff/3005/src/cmd/ld/ldpe.c#newcode304
> src/cmd/ld/ldpe.c:304: rp->add += obj->pesym[symindex].value;
> It seems to me that setting type = D_ADDR means that the symbol value will be
> added into the relocation anyhow.  So I don't know why this is right.
ld -r could generate multiple section symbols with the same name but different
values,
the file attached to issue 3322 demonstrated this problem; so my original code
doesn't
fix this real cause, but now I believe it is a correct fix for the problem.

[iant, 2012-03-16 14:04:00]

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

http://codereview.appspot.com/5823059/diff/10001/src/cmd/ld/ldpe.c#newcode306
src/cmd/ld/ldpe.c:306: if (obj->pesym[symindex].name[0] == '.')
In a PE file, a section symbol has storage class C_SECTION == 104, which is
better than checking the first character name of the name.  But a better test
here would be obj->pesym[symindex].value != 0 && rp->sym->value == 0.  In fact a
more general approach might be

rp->add += obj->pesym[symindex].value - rp->sym->value.

[rsc, 2012-03-16 16:23:43]

LGTM but wait for iant.

[minux1, 2012-03-16 17:17:12]

This is the relevant details of hh.o (you can reproduce it by using the
attachment in issue 3322)
(the file is produced by 'ld -r h2.o h.o', h2.c is a static function
referring the string "HAHA", while h.c
defines char* greeting = "hwllo")

SYMBOL TABLE:
[ 10](sec  3)(fl 0x00)(ty   0)(scl   3) (nx 1) 0x00000000 .rdata
[ 22](sec  3)(fl 0x00)(ty   0)(scl   3) (nx 1) 0x00000008 .rdata
AUX scnlen 0x6 nreloc 0 nlnno 0
[ 24](sec  2)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 _greeting

RELOCATION RECORDS FOR [.data]:
OFFSET   TYPE              VALUE
00000000 dir32             .rdata+0xfffffff8  # this refer to the 2nd .rdata

I don't know how to distinguish the second .rdata from other symbols
besides comparing the first
character against '.', besides, this
code<http://tip.golang.org/src/cmd/ld/ldpe.c#L409>also uses this to
determine if it is a section symbol.

On Fri, Mar 16, 2012 at 10:04 PM, <iant@golang.org> wrote:

>
>
http://codereview.appspot.com/**5823059/diff/10001/src/cmd/ld/**ldpe.c<http:/...
> File src/cmd/ld/ldpe.c (right):
>
> http://codereview.appspot.com/**5823059/diff/10001/src/cmd/ld/**
>
ldpe.c#newcode306<http://codereview.appspot.com/5823059/diff/10001/src/cmd/ld/ldpe.c#newcode306>
> src/cmd/ld/ldpe.c:306: if (obj->pesym[symindex].name[0] == '.')
> In a PE file, a section symbol has storage class C_SECTION == 104, which
> is better than checking the first character name of the name.  But a
> better test here would be obj->pesym[symindex].value != 0 &&
> rp->sym->value == 0.  In fact a more general approach might be
>
> rp->add += obj->pesym[symindex].value - rp->sym->value.
>
This is not correct, as I've found several symbols with
obj->pesym[symindex].value != rp->sym->value.
I add a print like this in the this spot:
if (obj->pesym[symindex].value != rp->sym->value)
print("! %s %s %d %d (%d)\n", rp->sym->name, obj->pesym[symindex].name, \
rp->sym->value, obj->pesym[symindex].value, obj->pesym[symindex].sclass);

when I do 'go test ../misc/cgo/test' on windows/386, part of the output is:
! __popcountsi2 ___popcountsi2 0 1976 (2)
! __chkstk_ms ___chkstk_ms 0 2036 (2)
! libcgo_sys_thread_start _libcgo_sys_thread_start 0 292 (2)
! crosscall_386 _crosscall_386 0 328 (2)
! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0 140 (3)
! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0 140 (3)

[iant2, 2012-03-16 19:16:17]

minux <minux.ma@gmail.com> writes:

> I don't know how to distinguish the second .rdata from other symbols
> besides comparing the first
> character against '.', besides, this
> code<http://tip.golang.org/src/cmd/ld/ldpe.c#L409>also uses this to
> determine if it is a section symbol.

OK, fair enough.


> On Fri, Mar 16, 2012 at 10:04 PM, <iant@golang.org> wrote:
>
>>
>>
http://codereview.appspot.com/**5823059/diff/10001/src/cmd/ld/**ldpe.c<http:/...
>> File src/cmd/ld/ldpe.c (right):
>>
>> http://codereview.appspot.com/**5823059/diff/10001/src/cmd/ld/**
>>
ldpe.c#newcode306<http://codereview.appspot.com/5823059/diff/10001/src/cmd/ld/ldpe.c#newcode306>
>> src/cmd/ld/ldpe.c:306: if (obj->pesym[symindex].name[0] == '.')
>> In a PE file, a section symbol has storage class C_SECTION == 104, which
>> is better than checking the first character name of the name.  But a
>> better test here would be obj->pesym[symindex].value != 0 &&
>> rp->sym->value == 0.  In fact a more general approach might be
>>
>> rp->add += obj->pesym[symindex].value - rp->sym->value.
>>
> This is not correct, as I've found several symbols with
> obj->pesym[symindex].value != rp->sym->value.
> I add a print like this in the this spot:
> if (obj->pesym[symindex].value != rp->sym->value)
> print("! %s %s %d %d (%d)\n", rp->sym->name, obj->pesym[symindex].name, \
> rp->sym->value, obj->pesym[symindex].value, obj->pesym[symindex].sclass);
>
> when I do 'go test ../misc/cgo/test' on windows/386, part of the output is:
> ! __popcountsi2 ___popcountsi2 0 1976 (2)
> ! __chkstk_ms ___chkstk_ms 0 2036 (2)
> ! libcgo_sys_thread_start _libcgo_sys_thread_start 0 292 (2)
> ! crosscall_386 _crosscall_386 0 328 (2)
> ! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0 140 (3)
> ! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0 140 (3)

I see, the difference is presumably whether the symbol is defined in the
same object file or not.  So how about

  if (obj->pesym[symindex].value != 0)
    rp->add += obj->pesym[symindex].value - rp->sym->value;

Ian

[minux1, 2012-03-17 05:10:26]

On Sat, Mar 17, 2012 at 3:16 AM, Ian Lance Taylor <iant@google.com> wrote:

> >> rp->add += obj->pesym[symindex].value - rp->sym->value.
> >>
> > This is not correct, as I've found several symbols with
> > obj->pesym[symindex].value != rp->sym->value.
> > I add a print like this in the this spot:
> > if (obj->pesym[symindex].value != rp->sym->value)
> > print("! %s %s %d %d (%d)\n", rp->sym->name, obj->pesym[symindex].name, \
> > rp->sym->value, obj->pesym[symindex].value, obj->pesym[symindex].sclass);
> >
> > when I do 'go test ../misc/cgo/test' on windows/386, part of the output
> is:
> > ! __popcountsi2 ___popcountsi2 0 1976 (2)
> > ! __chkstk_ms ___chkstk_ms 0 2036 (2)
> > ! libcgo_sys_thread_start _libcgo_sys_thread_start 0 292 (2)
> > ! crosscall_386 _crosscall_386 0 328 (2)
> > ! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0 140
> (3)
> > ! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0 140
> (3)
>
> I see, the difference is presumably whether the symbol is defined in the
> same object file or not.  So how about
>
>  if (obj->pesym[symindex].value != 0)
>    rp->add += obj->pesym[symindex].value - rp->sym->value;
>
But all the listed symbols satisfy this condition
(obj->pesym[symindex].value == 0 &&
rp->sym->value > 0), and none of them need adjustment in rp->add.
(In fact, I've just tried this approach, and got several runtime panics.)

[iant2, 2012-03-17 20:02:58]

On Mar 16, 2012 10:10 PM, "minux" <minux.ma@gmail.com> wrote:
>
>
> On Sat, Mar 17, 2012 at 3:16 AM, Ian Lance Taylor <iant@google.com> wrote:
>>
>> >> rp->add += obj->pesym[symindex].value - rp->sym->value.
>> >>
>> > This is not correct, as I've found several symbols with
>> > obj->pesym[symindex].value != rp->sym->value.
>> > I add a print like this in the this spot:
>> > if (obj->pesym[symindex].value != rp->sym->value)
>> > print("! %s %s %d %d (%d)\n", rp->sym->name,
obj->pesym[symindex].name, \
>> > rp->sym->value, obj->pesym[symindex].value,
obj->pesym[symindex].sclass);
>> >
>> > when I do 'go test ../misc/cgo/test' on windows/386, part of the
output is:
>> > ! __popcountsi2 ___popcountsi2 0 1976 (2)
>> > ! __chkstk_ms ___chkstk_ms 0 2036 (2)
>> > ! libcgo_sys_thread_start _libcgo_sys_thread_start 0 292 (2)
>> > ! crosscall_386 _crosscall_386 0 328 (2)
>> > ! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0
140 (3)
>> > ! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0
140 (3)
>>
>> I see, the difference is presumably whether the symbol is defined in the
>> same object file or not.  So how about
>>
>>  if (obj->pesym[symindex].value != 0)
>>    rp->add += obj->pesym[symindex].value - rp->sym->value;
>
> But all the listed symbols satisfy this condition
(obj->pesym[symindex].value == 0 &&
> rp->sym->value > 0), and none of them need adjustment in rp->add.
> (In fact, I've just tried this approach, and got several runtime panics.)

I was suggesting != 0.  I'm trying to catch the case of multiple
definitions of the symbol with different values.

Ian

[minux1, 2012-03-18 07:40:06]

On Sun, Mar 18, 2012 at 4:02 AM, Ian Lance Taylor <iant@google.com> wrote
>
>  On Mar 16, 2012 10:10 PM, "minux" <minux.ma@gmail.com> wrote:
> > On Sat, Mar 17, 2012 at 3:16 AM, Ian Lance Taylor <iant@google.com>
> wrote:
> >>
> >> >> rp->add += obj->pesym[symindex].value - rp->sym->value.
> >> >>
> >> > This is not correct, as I've found several symbols with
> >> > obj->pesym[symindex].value != rp->sym->value.
> >> > I add a print like this in the this spot:
> >> > if (obj->pesym[symindex].value != rp->sym->value)
> >> > print("! %s %s %d %d (%d)\n", rp->sym->name,
> obj->pesym[symindex].name, \
> >> > rp->sym->value, obj->pesym[symindex].value,
> obj->pesym[symindex].sclass);
> >> >
> >> > when I do 'go test ../misc/cgo/test' on windows/386, part of the
> output is:
> >> > ! __popcountsi2 ___popcountsi2 0 1976 (2)
> >> > ! __chkstk_ms ___chkstk_ms 0 2036 (2)
> >> > ! libcgo_sys_thread_start _libcgo_sys_thread_start 0 292 (2)
> >> > ! crosscall_386 _crosscall_386 0 328 (2)
> >> > ! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0
> 140 (3)
> >> > ! Z:\go.win.hg/pkg/windows_386/runtime/cgo.a(_all.o)(.text) .text 0
> 140 (3)
> >>
> >> I see, the difference is presumably whether the symbol is defined in the
> >> same object file or not.  So how about
> >>
> >>  if (obj->pesym[symindex].value != 0)
> >>    rp->add += obj->pesym[symindex].value - rp->sym->value;
> >
> > But all the listed symbols satisfy this condition
> (obj->pesym[symindex].value == 0 &&
> > rp->sym->value > 0), and none of them need adjustment in rp->add.
> > (In fact, I've just tried this approach, and got several runtime panics.)
>
> I was suggesting != 0.  I'm trying to catch the case of multiple
> definitions of the symbol with different values.
>
Sorry, my mistake in wording, I suppose to say "obj->pesym[symindex].value
> 0 && rp->sym->value == 0",
I changed the code print code to these:
if(obj->pesym[symindex].value != 0 && obj->pesym[symindex].name[0] != '.')
 print("! '%s' rp->sym:%d pesym:%d\n",
rp->sym->name,
 rp->sym->value,
obj->pesym[symindex].value);

And the full linker output when 'go test ../misc/cgo/test' is:
! 'callback' rp->sym:0 pesym:1884
! 'callGoFoo' rp->sym:0 pesym:1932
! 'IntoC' rp->sym:0 pesym:1940
! 'sleep' rp->sym:0 pesym:1848
! 'twoSleep' rp->sym:0 pesym:1948
! '__popcountsi2' rp->sym:0 pesym:1980
! '__popcountsi2' rp->sym:0 pesym:1980
! '__chkstk_ms' rp->sym:0 pesym:2040
! 'BackgroundSleep' rp->sym:0 pesym:160
! 'goFoo' rp->sym:0 pesym:44
! 'BackIntoGo' rp->sym:0 pesym:124
! 'sleep' rp->sym:0 pesym:1848
! '__popcount_tab' rp->sym:0 pesym:64
! '__popcount_tab' rp->sym:0 pesym:64
! '__popcount_tab' rp->sym:0 pesym:64
! '__popcount_tab' rp->sym:0 pesym:64
! 'libcgo_sys_thread_start' rp->sym:0 pesym:292
! 'crosscall_386' rp->sym:0 pesym:328

Oh, I think I know the problem here, nobody has changed rp->sym->value for
symbols
in COFF object files yet at this time.

[rsc, 2012-03-26 19:37:00]

Same as before: we're going to have to do Go 1 without fixing the
Windows cgo support.
It is too late to be making these changes to the linker.

Russ

[minux1, 2012-04-06 07:33:23]

ping.

[rsc, 2012-04-10 20:16:29]

Leaving for Ian so you can finish the discussion about the
right test to use here.  Ian is away until next week.

Russ

[iant, 2012-04-16 22:12:33]

LGTM

It's not really the right approach, but other code uses the same test for
section symbols, and this is better than what we have today.

[minux1, 2012-05-22 18:27:57]

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

cmd/ld: take section symbols' value into account for PE
    ld -r could generate multiple section symbols for the same section, 
but with different values, we have to take that into account.
    Fixes issue 3322.
    Part of issue 3261.
    For CL 5822049.

R=golang-dev, iant, rsc, iant
CC=golang-dev
http://codereview.appspot.com/5823059