code review 12938043: runtime: Fix build on older ARM

runtime: Fix build on older ARM

        The ARM external linking CL used BLX instructions in gcc assembler. Replace with BL to retain support on older ARM processors.

[elias.naur, 2013-08-14 17:25:45]

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

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

[rsc, 2013-08-14 17:45:05]

LGTM

[rsc, 2013-08-14 17:50:28]

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

runtime: Fix build on older ARM

        The ARM external linking CL used BLX instructions in gcc assembler.
Replace with BL to retain support on older ARM processors.

R=rsc
CC=golang-dev
https://codereview.appspot.com/12938043

Committer: Russ Cox <rsc@golang.org>

[cshapiro1, 2013-08-14 19:47:49]

https://codereview.appspot.com/12938043/diff/6001/src/pkg/runtime/cgo/gcc_arm.S
File src/pkg/runtime/cgo/gcc_arm.S (right):

https://codereview.appspot.com/12938043/diff/6001/src/pkg/runtime/cgo/gcc_arm...
src/pkg/runtime/cgo/gcc_arm.S:28: bl r5 // setmg(m, g)
I am not certain about this change.  There is no form of the BL instruction that
takes a register argument.  What does the assembler generate?

Elsewhere in the compiler we generate the BLX instruction for register indirect
calls so this change is not sufficient to support very old ARM processors.  BLX
is more efficient than the 2 instruction sequence it replaces as it is
recognized by the control flow prediction hardware.

[rsc, 2013-08-14 19:53:01]

On Wed, Aug 14, 2013 at 3:47 PM, <cshapiro@google.com> wrote:

>
> https://codereview.appspot.**com/12938043/diff/6001/src/**
>
pkg/runtime/cgo/gcc_arm.S<https://codereview.appspot.com/12938043/diff/6001/src/pkg/runtime/cgo/gcc_arm.S>
> File src/pkg/runtime/cgo/gcc_arm.S (right):
>
> https://codereview.appspot.**com/12938043/diff/6001/src/**
>
pkg/runtime/cgo/gcc_arm.S#**newcode28<https://codereview.appspot.com/12938043/diff/6001/src/pkg/runtime/cgo/gcc_arm.S#newcode28>
> src/pkg/runtime/cgo/gcc_arm.S:**28: bl r5 // setmg(m, g)
> I am not certain about this change.  There is no form of the BL
> instruction that takes a register argument.  What does the assembler
> generate?
>

This change is bogus. I replaced it with mov lr, pc; mov pc, r5 in a
subsequent change.


> Elsewhere in the compiler we generate the BLX instruction for register
> indirect calls so this change is not sufficient to support very old ARM
> processors.  BLX is more efficient than the 2 instruction sequence it
> replaces as it is recognized by the control flow prediction hardware.
>

It appears that while Go requires that the ARM core support the BLX
instruction, it does not convey this fact to gcc/gas when compiling this
file. You can see the build failure here:
http://build.golang.org/log/5459fbb12ec3f55eab6644a625814f99794e80be.

Russ

[cshapiro1, 2013-08-14 19:57:47]

On Wed, Aug 14, 2013 at 12:52 PM, Russ Cox <rsc@golang.org> wrote:

> This change is bogus. I replaced it with mov lr, pc; mov pc, r5 in a
> subsequent change.
>

Thanks, I eventually noticed that after I got caught up with my golang-dev
mail.


> It appears that while Go requires that the ARM core support the BLX
> instruction, it does not convey this fact to gcc/gas when compiling this
> file. You can see the build failure here:
> http://build.golang.org/log/5459fbb12ec3f55eab6644a625814f99794e80be.
>

If this is just a matter of providing the right flag to gas I am happy to
provide the CL.  What do you think?

[rsc, 2013-08-14 20:05:04]

On Wed, Aug 14, 2013 at 3:57 PM, Carl Shapiro <cshapiro@google.com> wrote:

> On Wed, Aug 14, 2013 at 12:52 PM, Russ Cox <rsc@golang.org> wrote:
>
>> This change is bogus. I replaced it with mov lr, pc; mov pc, r5 in a
>> subsequent change.
>>
>
> Thanks, I eventually noticed that after I got caught up with my golang-dev
> mail.
>
>
>> It appears that while Go requires that the ARM core support the BLX
>> instruction, it does not convey this fact to gcc/gas when compiling this
>> file. You can see the build failure here:
>> http://build.golang.org/log/5459fbb12ec3f55eab6644a625814f99794e80be.
>>
>
> If this is just a matter of providing the right flag to gas I am happy to
> provide the CL.  What do you think?
>

Sure. A good place for it would be src/cmd/go/build.go's gccArchArgs.

Russ

[cshapiro1, 2013-08-14 20:07:49]

On Wed, Aug 14, 2013 at 1:05 PM, Russ Cox <rsc@golang.org> wrote:

> Sure. A good place for it would be src/cmd/go/build.go's gccArchArgs.
>

The fix seems to be adding -march=armv5t to those flags.  I will send out a
change later.