Re: [PATCH] [RFC] PR target/52813 and target/11807

[Segher Boessenkool <segher@kernel.crashing.org>]

On Fri, Dec 14, 2018 at 01:49:40PM +0000, Richard Sandiford wrote:
> > OK, I read PR52813 too, but I'm not sure to fully understand the new status.
> > My understanding is that since this patch was committed, if an asm statement
> > clobbers sp, it is now allowed to actually declare it as clobber (this patch
> > generates an error in such a case).
> > So the user is now expected to lie to the compiler when writing to
> > this kind of register (sp, pic register), by not declaring it as "clobber"?
> 
> The user isn't expected to lie.  The point is that GCC simply doesn't
> support asms that leave the stack pointer with a different value from
> before, and IMO never has.  If that happened to work in some cases then
> it was purely an accident.

Yup.


It now errors for

void f(void)
{
      asm("ohmy" ::: "sp");
}

but not for

void f(void)
{
        register long x asm("sp");
        asm("ohmy %0" : "=r"(x));
}

which is the same problem.

(I would be happier if it was a warning instead of an error btw, since
there apparently is existing code that uses a clobber of sp, and GCC has
always worked with that, accidentally or not).

> The PRs also show disagreement about what GCC should do for an asm like
> that.  The asm in PR52813 temporarily changed the stack pointer and the
> bug was that GCC didn't restore the original value afterwards.  The asm
> in PR77904 was trying to set the stack pointer to an entirely new value
> and the bug was the GCC did restore the original value afterwards,
> defeating the point.
> 
> This wouldn't be the first time that there's disagreement about what
> the behaviour should be.  But IMO we can't support either reliably.
> Spilling sp is dangerous in general because we might need the stack
> for the reload, or we might accidentally try to reload something else
> before restoring the stack pointer.  And continuing with a new sp
> provided by the asm could lead to all sorts of problems.  (AIUI, the
> point of PR77904 was that it would also be wrong for GCC to set up a
> frame pointer and restore the sp from that frame pointer on function
> exit.  The new sp value was supposed to survive.  So the answer isn't
> simply "use a frame pointer".)

Inline asm cannot do things that are not allowed by the ABI, or that
touch other things in the execution environment you shouldn't touch.

Apparently some people really try to clobber sp, and this new error
will help them a bit.  I don't know how useful that is, is it better
to scare people away from using inline asm?  It might well be safer
for everyone...


Segher