Hi,

while I looked closely at the asm statement in the gdb,
I realized that the SP clobber forces the function to use
the frame pointer, and prevents the red zone.  That
makes the push / pop sequence in the asm statement safe
to use, as long as the stack is restored to the original
value.  That can be a quite useful feature.  And that might
have been the reason why the rsp clobber was chosen in the
first place.

This seems to work for all targets, but it started to work
this way with gcc-6, all versions before that do ignore
this clobber stmt (as confirmed by godbolt).

The clobber stmt make the LRA register allocator switch
frame_pointer_needed to 1, and therefore in all likelihood,
all targets should use that consistently.

On 12/17/18 12:47 PM, Richard Sandiford wrote:
> Dimitar Dimitrov <dimitar@dinux.eu> writes:
>> On Sun, Dec 16 2018 at 14:36:26 EET Bernd Edlinger wrote:
>>> Hi,
>>>
>>> if I understood that right, then clobbering sp is and has always been
>>> ignored.
> 
> PR77904 was about the clobber not being ignored, so the behaviour
> hasn't been consistent.
> 

I think 77904 was a fall-out from the change in the LRA register allocator.
The patch referenced in the PR does simply honor frame_pointer_needed,
which changed with gcc-6, and caused a regression on arm.

> I'm also not sure it was always ignored in recent sources.  The clobber
> does get added to the associated rtl insn, and it'd be surprising if
> that never had an effect.
> 
>>> If that is right, then I would much prefer a warning, that says exactly
>>> that, because that would also help to understand why removing that clobber
>>> statement is safe even for old gcc versions.
> 
> If the asm does leave sp with a different value, then it's never been safe,
> regardless of the gcc version.  That's why an error seems more appropriate.
> 
>> Thank you. Looks like general consensus is to have a warning. See attached
>> patch that switches the error to a warning.
> 
> I don't think there's a good reason to treat this differently from the
> preexisting PIC register error.  If the argument for making it a warning
> rather than an error is that the asm might happen to work by accident,
> then the same is true for the PIC register.
> 

In the light of my findings, I believe with a good warning message that
explains that the SP needs to be restored to the previous value, that
is a useful feature, that enables the asm statement to push temporary
values on the stack which would not be safe otherwise.

Therefore I propose not to rip it out at this time.
See my proposed patch.  What do you think?

Is it OK?


Thanks
Bernd.