[PATCH 2/4] msm: scm: Fix improper register assignment

[Nicolas Pitre]

On Sat, 26 Feb 2011, David Brown wrote:

> On Sat, Feb 26 2011, Russell King - ARM Linux wrote:
> 
> > On Fri, Feb 25, 2011 at 09:09:05PM -0800, Saravana Kannan wrote:
> > One way to look at it is that if you specify a value for r0, assign it,
> > and then call a function, how do you expect the r0 value to be preserved?
> > r0 will be corrupted by the called function as its used to pass arg0 and
> > the return value.
> 
> > I'm surprised the compiler didn't spit out an error.

Me too.  The compiler should have moved the content of r0 somewhere else 
before the function call, and restore it back into r0 if necessary 
before the point where the corresponding variable is used again.  Or at 
least issue a warning if it can't do that.

> The gcc docs say:
> 
>    * Local register variables in specific registers do not reserve the
>      registers, except at the point where they are used as input or
>      output operands in an `asm' statement and the `asm' statement
>      itself is not deleted.  The compiler's data flow analysis is
>      capable of determining where the specified registers contain live
>      values, and where they are available for other uses.  Stores into
>      local register variables may be deleted when they appear to be
>      dead according to dataflow analysis.  References to local register
>      variables may be deleted or moved or simplified.
> 
> which would suggest that it should at least detect that it can't keep
> the value in r0.  What it seems to do is detect that the value can't be
> in the register, so it never bothers putting it there in the first
> place.

Right.  A minimal test case may look like this if someone feels like 
filling a gcc bug report:

extern int foo(int x);

int bar(int x)
{
	register int a asm("r0") = 1;
	x = foo(x);
	asm ("add %0, %1, %2" : "=r" (x) : "r" (a), "r" (x));
	return x;
}

And the produced code is:

bar:
        stmfd   sp!, {r3, lr}
        bl      foo
#APP
        add r0, r0, r0
        ldmfd   sp!, {r3, pc}

So this is clearly bogus.

> In any case, fortunately it works with the fix.

Please add a comment in your patch to explain the issue.


Nicolas