code review 5488074: ld: allow for IMAGE_REL_AMD64_ADDR32NB relocation type

ld: allow for IMAGE_REL_AMD64_ADDR32NB relocation type

enable cgo again on windows/amd64

[brainman, 2011-12-14 04:41:30]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-12-14 04:51:30]

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

http://codereview.appspot.com/5488074/diff/1002/src/cmd/ld/ldpe.c#newcode285
src/cmd/ld/ldpe.c:285: case IMAGE_REL_AMD64_ADDR32NB:
I would have expected this next to the 
case IMAGE_REL_AMD64_ADDR32:, since the
I386 DIR32NB and DIR32 are next to each other.
Does this work?  Does it work if you move it down there?

[brainman, 2011-12-14 05:26:23]

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

http://codereview.appspot.com/5488074/diff/1002/src/cmd/ld/ldpe.c#newcode285
src/cmd/ld/ldpe.c:285: case IMAGE_REL_AMD64_ADDR32NB:
On 2011/12/14 04:51:30, rsc wrote:
> I would have expected this next to the 
> case IMAGE_REL_AMD64_ADDR32:, since the
> I386 DIR32NB and DIR32 are next to each other.

I do not know much about all these formats. But here is the manual:
http://kishorekumar.net/pecoff_v8.1.htm.

IMAGE_REL_AMD64_ADDR32 - The 32-bit VA of the relocation target
IMAGE_REL_AMD64_ADDR32NB - The 32-bit address without an image base (RVA)

so these two look different to me. I didn't see any of 
IMAGE_REL_AMD64_ADDR32 in the misc/cgo/life objects anyway.

> Does this work?  

life test builds and runs successfully.

> Does it work if you move it down there?

Looks like it works regardless. I can put it anywhere :-)

Would you like to make a pick?

Should we cc Wei?

[rsc, 2011-12-14 05:31:58]

Wei,

Do you know the right place to put this new case in the amd64 relocation code?

Thanks.
Russ

[vcc, 2011-12-14 05:39:02]

2011/12/14 Russ Cox <rsc@golang.org>:
> Wei,
>
> Do you know the right place to put this new case in the amd64 relocation code?

Alex is right,  and please make IMAGE_REL_AMD64_ADDR32 with
IMAGE_REL_AMD64_ADDR32NB:
--- a/src/cmd/ld/ldpe.c Wed Dec 14 13:17:48 2011 +1100
+++ b/src/cmd/ld/ldpe.c Wed Dec 14 13:33:19 2011 +0800
@@ -282,6 +282,8 @@
                                        diag("%s: unknown relocation type %d;",
pn, type);
                                case IMAGE_REL_I386_REL32:
                                case IMAGE_REL_AMD64_REL32:
+                               case IMAGE_REL_AMD64_ADDR32: // R_X86_64_PC32
+                               case IMAGE_REL_AMD64_ADDR32NB:
                                        rp->type = D_PCREL;
                                        rp->add = le32(rsect->base+rp->off);
                                        break;
@@ -291,10 +293,6 @@
                                        // load addend from image
                                        rp->add = le32(rsect->base+rp->off);
                                        break;
-                               case IMAGE_REL_AMD64_ADDR32: // R_X86_64_PC32
-                                       rp->type = D_PCREL;
-                                       rp->add += 4;
-                                       break;
                                case IMAGE_REL_AMD64_ADDR64: // R_X86_64_64
                                        rp->siz = 8;
                                        rp->type = D_ADDR;

>
> Thanks.
> Russ

[brainman, 2011-12-14 05:43:07]

On 2011/12/14 05:39:02, vcc wrote:
> 
> Alex is right,  and please make IMAGE_REL_AMD64_ADDR32 with
> IMAGE_REL_AMD64_ADDR32NB:

Will do. Thank you.

Do you know how I could test my change properly? misc/cgo/life is the only cgo
program that I ever built on windows, but it works regardless of where I put
IMAGE_REL_AMD64_ADDR32NB. And it does not have any of IMAGE_REL_AMD64_ADDR32 at
all.

Alex

[brainman, 2011-12-14 06:07:51]

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

Please take another look.

[vcc, 2011-12-14 06:39:32]

2011/12/14  <alex.brainman@gmail.com>:
> On 2011/12/14 05:39:02, vcc wrote:
>
>> Alex is right,  and please make IMAGE_REL_AMD64_ADDR32 with
>> IMAGE_REL_AMD64_ADDR32NB:
>
>
> Will do. Thank you.
>
> Do you know how I could test my change properly? misc/cgo/life is the
> only cgo program that I ever built on windows, but it works regardless
> of where I put IMAGE_REL_AMD64_ADDR32NB. And it does not have any of
> IMAGE_REL_AMD64_ADDR32 at all.

I also don't see IMAGE_REL_AMD64_ADDR32 (maybe seen but forget), I
think load  addend from image is OK, don't need explicit add 4.

>
> Alex
>
> http://codereview.appspot.com/5488074/

[rsc, 2011-12-14 22:15:15]

LGTM

[brainman, 2011-12-15 23:32:21]

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

ld: allow for IMAGE_REL_AMD64_ADDR32NB relocation type

enable cgo again on windows/amd64

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