code review 91500046: cmd/ld: abort if (32-bit) address relocation is negativ...

cmd/ld: abort if (32-bit) address relocation is negative on amd64.
Update issue 7980
This CL make the linker abort for the example program. For Go 1.4,
we need to find a general way to handle large memory model programs.

[minux1, 2014-05-15 23:20:14]

Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),

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

[dfc, 2014-05-15 23:38:01]

LGTM. I think this is a reasonable way to ensure we don't generate
broken programs.

As I understand it there is an existing problem with the
compier/linker that if I say

package p

var a [1<<24]int

This is already not handled efficiently by the compiler/linker, so the
problem we are protecting against is not the common (I hope)

On Fri, May 16, 2014 at 9:20 AM,  <minux.ma@gmail.com> wrote:
> Reviewers: iant,
>
> Message:
> Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/ld: abort if (32-bit) address relocation is negative on amd64.
> Update issue 7980
> This CL make the linker abort for the example program. For Go 1.4,
> we need to find a general way to handle large memory model programs.
>
> Please review this at https://codereview.appspot.com/91500046/
>
> Affected files (+6, -0 lines):
>   M src/cmd/ld/data.c
>
>
> Index: src/cmd/ld/data.c
> ===================================================================
> --- a/src/cmd/ld/data.c
> +++ b/src/cmd/ld/data.c
> @@ -243,6 +243,12 @@
>                                 break;
>                         }
>                         o = symaddr(r->sym) + r->add;
> +
> +                       // On amd64, offset will be sign-extended, so it is
> impossible to access
> +                       // more than 2GB of static data; fail at link time
> is better than fail at
> +                       // runtime. See http://golang.org/issue/7980.
> +                       if((int32)o < 0 && thechar == '6')
> +                               diag("non-pc-relative relocation address is
> too big: %#llux", o);
>                         break;
>                 case R_CALL:
>                 case R_PCREL:
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[josharian, 2014-05-15 23:53:25]

LGTM, at least for 1.3

Thanks, minux.

https://codereview.appspot.com/91500046/diff/40001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

https://codereview.appspot.com/91500046/diff/40001/src/cmd/ld/data.c#newcode251
src/cmd/ld/data.c:251: diag("non-pc-relative relocation address is too big:
%#llux", o);
Add errorexit() -- once this happens, there's no point in continuing.

[iant, 2014-05-15 23:53:45]

https://codereview.appspot.com/91500046/diff/40001/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

https://codereview.appspot.com/91500046/diff/40001/src/cmd/ld/data.c#newcode250
src/cmd/ld/data.c:250: if((int32)o < 0 && thechar == '6')
I think we can future-proof this a bit.

if(siz == 4 && PtrSize > 4 && (int32)o < 0)

[minux1, 2014-05-16 00:06:57]

Hello iant@golang.org, dave@cheney.net, josharian@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[minux1, 2014-05-16 00:08:09]

On May 15, 2014 7:53 PM, <josharian@gmail.com> wrote:
> https://codereview.appspot.com/91500046/diff/40001/src/cmd/ld/data.c
> File src/cmd/ld/data.c (right):
>
>
https://codereview.appspot.com/91500046/diff/40001/src/cmd/ld/data.c#newcode251
> src/cmd/ld/data.c:251: diag("non-pc-relative relocation address is too
> big: %#llux", o);
> Add errorexit() -- once this happens, there's no point in continuing.
yes, and this error most likely will lead to cascaded errors so better exit
now.

[josharian, 2014-05-16 00:08:42]

> src/cmd/ld/data.c:250: if((int32)o < 0 && thechar == '6')
> I think we can future-proof this a bit.
> 
> if(siz == 4 && PtrSize > 4 && (int32)o < 0)

Maybe I'm missing something, but the use of signed pointers seems
architecture-specific, so checking explicitly for amd64 seems right here.

Perhaps we could future-proof by adding a diagnostic test instead. It's not
clear to me what that should look like, though, given that we're punting for
now.

[minux1, 2014-05-16 00:13:03]

On May 15, 2014 7:53 PM, <iant@golang.org> wrote:
> https://codereview.appspot.com/91500046/diff/40001/src/cmd/ld/data.c
> File src/cmd/ld/data.c (right):
>
>
https://codereview.appspot.com/91500046/diff/40001/src/cmd/ld/data.c#newcode250
> src/cmd/ld/data.c:250: if((int32)o < 0 && thechar == '6')
> I think we can future-proof this a bit.
>
> if(siz == 4 && PtrSize > 4 && (int32)o < 0)
adopted. although i think it's fairly certain that all 64-bit architectures
will sign-extend a offset, this probably won't work on aarch64 (as the
addend contains the actual instruction). however we need a better way to
handle that case anyway.

[iant, 2014-05-16 00:39:06]

On Thu, May 15, 2014 at 5:08 PM,  <josharian@gmail.com> wrote:
>> src/cmd/ld/data.c:250: if((int32)o < 0 && thechar == '6')
>> I think we can future-proof this a bit.
>
>
>> if(siz == 4 && PtrSize > 4 && (int32)o < 0)
>
>
> Maybe I'm missing something, but the use of signed pointers seems
> architecture-specific, so checking explicitly for amd64 seems right
> here.

Each new 64-bit architecture with 32-bit relocs will need this
condition checked and handled as appropriate.  Just checking for amd64
as in the original patch means that other architectures may silently
accept a bad reloc.  We could add a switch and crash on an unknown
case but that seems like overkill at present.  It seems OK to me to
leave this as I wrote it and then modify it if we later find that we
need to.

Ian

[iant, 2014-05-16 00:39:30]

LGTM

[josharian, 2014-05-16 00:50:47]

> >> src/cmd/ld/data.c:250: if((int32)o < 0 && thechar == '6')
> >> I think we can future-proof this a bit.
> >
> >
> >> if(siz == 4 && PtrSize > 4 && (int32)o < 0)
> >
> >
> > Maybe I'm missing something, but the use of signed pointers seems
> > architecture-specific, so checking explicitly for amd64 seems right
> > here.
> 
> Each new 64-bit architecture with 32-bit relocs will need this
> condition checked and handled as appropriate.  Just checking for amd64
> as in the original patch means that other architectures may silently
> accept a bad reloc.  We could add a switch and crash on an unknown
> case but that seems like overkill at present.  It seems OK to me to
> leave this as I wrote it and then modify it if we later find that we
> need to.

Ahhh. Makes sense; thanks for the explanation.

[rsc, 2014-05-20 02:39:44]

*** Submitted as https://code.google.com/p/go/source/detail?r=793272b5aeef ***

cmd/ld: abort if (32-bit) address relocation is negative on amd64.
Update issue 7980
This CL make the linker abort for the example program. For Go 1.4,
we need to find a general way to handle large memory model programs.

LGTM=dave, josharian, iant
R=iant, dave, josharian
CC=golang-codereviews
https://codereview.appspot.com/91500046

Committer: Russ Cox <rsc@golang.org>