Code review - Issue 91500046: code review 91500046: cmd/ld: abort if (32-bit) address relocation is negativ...https://codereview.appspot.com/2014-05-20T02:39:44+00:00rietveld
Message from unknown
2014-05-15T23:14:06+00:00minux1urn:md5:d20684bc5321e7c26f0a26908249d5d9
Message from unknown
2014-05-15T23:14:09+00:00minux1urn:md5:ba3ac23d4ba7f745ead02f990a6885e6
Message from unknown
2014-05-15T23:20:11+00:00minux1urn:md5:dfda8e2aa9726f44a547c5611027d490
Message from minux.ma@gmail.com
2014-05-15T23:20:14+00:00minux1urn:md5:c5e2d910fb781391149a6cb6a235d93e
Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go
Message from dave@cheney.net
2014-05-15T23:38:01+00:00dfcurn:md5:cb35ede55b9e2989f5c5bd0bd07333e5
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.
Message from josharian@gmail.com
2014-05-15T23:53:25+00:00josharianurn:md5:e6bb976e65f3932e7c99b62f6006d339
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.
Message from iant@golang.org
2014-05-15T23:53:45+00:00ianturn:md5:7cf1d8ab5f26e7b8e51607721e9310d6
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)
Message from unknown
2014-05-16T00:04:34+00:00minux1urn:md5:f5ed94ed3cabba3e9c7a916da8953bfb
Message from unknown
2014-05-16T00:06:53+00:00minux1urn:md5:0b99f45b555a8d31bb1ac00b150ed05d
Message from minux.ma@gmail.com
2014-05-16T00:06:57+00:00minux1urn:md5:cad80f3ba17bb184de06d725d4a888a8
Hello iant@golang.org, dave@cheney.net, josharian@gmail.com (cc: golang-codereviews@googlegroups.com),
Please take another look.
Message from minux.ma@gmail.com
2014-05-16T00:08:09+00:00minux1urn:md5:938459d923b525b54c5bf5795dd2abc1
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.
Message from josharian@gmail.com
2014-05-16T00:08:42+00:00josharianurn:md5:94c6cf7cc23aff9c7bc12f9be9715b8b
> 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.
Message from minux.ma@gmail.com
2014-05-16T00:13:03+00:00minux1urn:md5:fff482f4ab79a580fa56107675ddf5ee
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.
Message from iant@golang.org
2014-05-16T00:39:06+00:00ianturn:md5:25b13cd01842bd56cd63befdfcb2806c
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
Message from iant@golang.org
2014-05-16T00:39:30+00:00ianturn:md5:beadbc7b4e6575e56747b9b33520fd36
LGTM
Message from josharian@gmail.com
2014-05-16T00:50:47+00:00josharianurn:md5:ea26d59b1fc1dcf4b8517d442e182532
> >> 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.
Message from rsc@golang.org
2014-05-20T02:39:44+00:00rscurn:md5:b3e50e6199035531d6964a003eb226cb
*** 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>