code review 6501099: runtime: arm: abort if VFPv3 support missing

runtime: arm: abort if VFPv3 support missing

Fixes issue 3456.

This proposal is a reformulation of CL 5987063. This CL resets
the default GOARM value to 6 and allows the use of the VFPv3
optimisation if GOARM=7. Binaries built with this CL in place
will abort if GOARM=7 was used and the target host does not
support VFPv3.

[dfc, 2012-09-07 11:01:02]

Hello minux.ma@gmail.com, rsc@golang.org, ajstarks@gmail.com (cc:
golang-dev@googlegroups.com),

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

[ajstarks, 2012-09-08 06:15:43]

Tested with the raspberry pi: (Raspbian distro, hardware float)

pi@pi ~/go/src/pkg/runtime $ uname -a
Linux pi 3.2.27+ #102 PREEMPT Sat Sep 1 01:00:50 BST 2012 armv6l GNU/Linux
pi@pi ~/go/src/pkg/runtime $ go test -c && ./runtime.test
PASS
pi@pi ~/go/src/pkg/runtime $ GOARM=5 go test -c && ./runtime.test
PASS
pi@pi ~/go/src/pkg/runtime $ GOARM=7 go test -c && ./runtime.test
runtime: this CPU has no VFPv3 floating point hardware, so it cannot run
this GOARM=7 binary. Recompile using GOARM=6.




On Sep 7, 2012, at 7:01 AM, dave@cheney.net wrote:

> Reviewers: minux, rsc, ajstarks,
> 
> Message:
> Hello minux.ma@gmail.com, rsc@golang.org, ajstarks@gmail.com (cc:
> golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go
> 
> 
> Description:
> runtime: arm: abort if VFPv3 support missing
> 
> Fixes issue 3456.
> 
> This proposal is a reformulation of CL 5987063. This CL resets
> the default GOARM value to 6 and allows the use of the VFPv3
> optimisation if GOARM=7. Binaries built with this CL in place
> will abort if GOARM=7 was used and the target host does not
> support VFPv3.
> 
> Please review this at http://codereview.appspot.com/6501099/
> 
> Affected files:
>  M src/cmd/5l/asm.c
>  M src/cmd/5l/l.h
>  M src/cmd/5l/obj.c
>  M src/pkg/runtime/signal_linux_arm.c
> 
> 
> Index: src/cmd/5l/asm.c
> ===================================================================
> --- a/src/cmd/5l/asm.c
> +++ b/src/cmd/5l/asm.c
> @@ -2211,7 +2211,9 @@
> int
> chipzero(Ieee *e)
> {
> -	if(e->l != 0 || e->h != 0)
> +	// if this procedure returns 0, then 5l will use the "vmov (imm)"
> +	// instruction which only exists on VFPv3. We use GOARM=7 to gate it.
> +	if(goarm < 7 || e->l != 0 || e->h != 0)
> 		return -1;
> 	return 0;
> }
> @@ -2222,6 +2224,11 @@
> 	int n;
> 	ulong h;
> 
> +	// if this procedure returns non-negative number 5l will use the "vmov
(imm)"
> +	// instruction which only exists on VFPv3. We use GOARM=7 to gate it.
> +	if(goarm < 7)
> +		goto no;
> +
> 	if(e->l != 0 || (e->h&0xffff) != 0)
> 		goto no;
> 	h = e->h & 0x7fc00000;
> Index: src/cmd/5l/l.h
> ===================================================================
> --- a/src/cmd/5l/l.h
> +++ b/src/cmd/5l/l.h
> @@ -316,6 +316,8 @@
> EXTERN	Prog*	blitrl;
> EXTERN	Prog*	elitrl;
> 
> +EXTERN	int	goarm; // default to 6
> +
> void	initdiv(void);
> EXTERN	Prog*	prog_div;
> EXTERN	Prog*	prog_divu;
> Index: src/cmd/5l/obj.c
> ===================================================================
> --- a/src/cmd/5l/obj.c
> +++ b/src/cmd/5l/obj.c
> @@ -93,7 +93,7 @@
> 	if(p != nil)
> 		goarm = atoi(p);
> 	else
> -		goarm = 7;
> +		goarm = 6;
> 	if(goarm == 5)
> 		debug['F'] = 1;
> 
> Index: src/pkg/runtime/signal_linux_arm.c
> ===================================================================
> --- a/src/pkg/runtime/signal_linux_arm.c
> +++ b/src/pkg/runtime/signal_linux_arm.c
> @@ -148,6 +148,7 @@
> #define AT_HWCAP	16 // introduced in at least 2.6.11
> #define AT_RANDOM	25 // introduced in 2.6.29
> #define HWCAP_VFP	(1 << 6)
> +#define HWCAP_VFPv3	(1 << 13)
> static uint32 runtime·randomNumber;
> uint8  runtime·armArch = 6;	// we default to ARMv6
> uint32 runtime·hwcap;	// set by setup_auxv
> @@ -161,6 +162,11 @@
> 		runtime·printf("this GOARM=%d binary. Recompile using GOARM=5.\n",
runtime·goarm);
> 		runtime·exit(1);
> 	}
> +	if(runtime·goarm > 6 && !(runtime·hwcap & HWCAP_VFPv3)) {
> +		runtime·printf("runtime: this CPU has no VFPv3 floating point hardware, so
it cannot run\n");
> +		runtime·printf("this GOARM=%d binary. Recompile using GOARM=6.\n",
runtime·goarm);
> +		runtime·exit(1);
> +	}
> }
> 
> #pragma textflag 7
> 
> 

[dfc, 2012-09-11 23:08:25]

ping

[minux1, 2012-09-16 08:04:18]

LGTM with some minor comment suggestions.

sorry for the late review.

http://codereview.appspot.com/6501099/diff/7/src/cmd/5l/asm.c
File src/cmd/5l/asm.c (right):

http://codereview.appspot.com/6501099/diff/7/src/cmd/5l/asm.c#newcode2228
src/cmd/5l/asm.c:2228: // instruction which only exists on VFPv3. We use GOARM=7
to gate it.
do we need to mention the non-negative return value here?
i think it suffices to say we use GOARM=7 to gate vmov (imm).

http://codereview.appspot.com/6501099/diff/7/src/cmd/5l/l.h
File src/cmd/5l/l.h (right):

http://codereview.appspot.com/6501099/diff/7/src/cmd/5l/l.h#newcode319
src/cmd/5l/l.h:319: EXTERN	int	goarm; // default to 6
i suggest we don't add this comment, then we don't
need to keep this in sync with the initialization code.

http://codereview.appspot.com/6501099/diff/7/src/pkg/runtime/signal_linux_arm.c
File src/pkg/runtime/signal_linux_arm.c (right):

http://codereview.appspot.com/6501099/diff/7/src/pkg/runtime/signal_linux_arm...
src/pkg/runtime/signal_linux_arm.c:150: #define HWCAP_VFP	(1 << 6)
while we're at it, please add // introduced in at least 2.6.11

ref: http://lxr.linux.no/linux+v2.6.11/include/asm-arm/procinfo.h

http://codereview.appspot.com/6501099/diff/7/src/pkg/runtime/signal_linux_arm...
src/pkg/runtime/signal_linux_arm.c:151: #define HWCAP_VFPv3	(1 << 13)
// introduced in 2.6.30

note for reviewers:
this is ok, because you can't use VFPv3 without kernel's support.

ref: http://lxr.linux.no/linux+v2.6.30/arch/arm/include/asm/hwcap.h#L20

[dfc, 2012-09-16 15:05:50]

Thank you very much for your comments. PTAL.

http://codereview.appspot.com/6501099/diff/7/src/cmd/5l/asm.c
File src/cmd/5l/asm.c (right):

http://codereview.appspot.com/6501099/diff/7/src/cmd/5l/asm.c#newcode2228
src/cmd/5l/asm.c:2228: // instruction which only exists on VFPv3. We use GOARM=7
to gate it.
On 2012/09/16 08:04:18, minux wrote:
> do we need to mention the non-negative return value here?
> i think it suffices to say we use GOARM=7 to gate vmov (imm).

Done.

http://codereview.appspot.com/6501099/diff/7/src/cmd/5l/l.h
File src/cmd/5l/l.h (right):

http://codereview.appspot.com/6501099/diff/7/src/cmd/5l/l.h#newcode319
src/cmd/5l/l.h:319: EXTERN	int	goarm; // default to 6
On 2012/09/16 08:04:18, minux wrote:
> i suggest we don't add this comment, then we don't
> need to keep this in sync with the initialization code.

Done.

http://codereview.appspot.com/6501099/diff/7/src/pkg/runtime/signal_linux_arm.c
File src/pkg/runtime/signal_linux_arm.c (right):

http://codereview.appspot.com/6501099/diff/7/src/pkg/runtime/signal_linux_arm...
src/pkg/runtime/signal_linux_arm.c:150: #define HWCAP_VFP	(1 << 6)
On 2012/09/16 08:04:18, minux wrote:
> while we're at it, please add // introduced in at least 2.6.11
> 
> ref: http://lxr.linux.no/linux+v2.6.11/include/asm-arm/procinfo.h

Done.

http://codereview.appspot.com/6501099/diff/7/src/pkg/runtime/signal_linux_arm...
src/pkg/runtime/signal_linux_arm.c:151: #define HWCAP_VFPv3	(1 << 13)
On 2012/09/16 08:04:18, minux wrote:
> // introduced in 2.6.30
> 
> note for reviewers:
> this is ok, because you can't use VFPv3 without kernel's support.
> 
> ref: http://lxr.linux.no/linux+v2.6.30/arch/arm/include/asm/hwcap.h#L20

Done.

[dfc, 2012-09-16 15:05:59]

Hello minux.ma@gmail.com, rsc@golang.org, ajstarks@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[minux1, 2012-09-16 17:05:37]

LGTM, please wait for rsc.

[rsc, 2012-09-17 21:13:17]

LGTM

[dfc, 2012-09-17 23:55:16]

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

runtime: arm: abort if VFPv3 support missing

Fixes issue 3456.

This proposal is a reformulation of CL 5987063. This CL resets
the default GOARM value to 6 and allows the use of the VFPv3
optimisation if GOARM=7. Binaries built with this CL in place
will abort if GOARM=7 was used and the target host does not
support VFPv3.

R=minux.ma, rsc, ajstarks
CC=golang-dev
http://codereview.appspot.com/6501099