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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr52813.diff --]
[-- Type: text/x-patch; name="patch-pr52813.diff", Size: 2726 bytes --]

2018-12-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* cfgexpand.c (asm_clobber_reg_is_valid): Emit only a warning together
	with an information message when the stack pointer is clobbered.

testsuite:
2018-12-18  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gcc.target/i386/pr52813.c: Adjust test.

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c	(revision 267164)
+++ gcc/cfgexpand.c	(working copy)
@@ -2854,6 +2854,7 @@ tree_conflicts_with_clobbers_p (tree t, HARD_REG_S
    asm clobber operand.  Some HW registers cannot be
    saved/restored, hence they should not be clobbered by
    asm statements.  */
+
 static bool
 asm_clobber_reg_is_valid (int regno, int nregs, const char *regname)
 {
@@ -2872,11 +2873,23 @@ asm_clobber_reg_is_valid (int regno, int nregs, co
       error ("PIC register clobbered by %qs in %<asm%>", regname);
       is_valid = false;
     }
-  /* Clobbering the STACK POINTER register is an error.  */
+  /* Clobbering the STACK POINTER register is likely an error.
+     However it is useful to force the use of frame pointer and prevent
+     the use of red zone.  Thus without this clobber, pushing temporary
+     values onto the stack might clobber the red zone or make stack based
+     memory references invalid.  */
   if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM))
     {
-      error ("Stack Pointer register clobbered by %qs in %<asm%>", regname);
-      is_valid = false;
+      if (warning (0, "Stack Pointer register clobbered by %qs in %<asm%>",
+		   regname))
+	{
+	  inform (input_location,
+		  "This does likely not do what you would expect."
+		  " The Stack Pointer register still needs to be restored to"
+		  " the previous value, however it is safe to push values onto"
+		  " the stack, when they are popped again from the stack"
+		  " before the asm statement terminates");
+	}
     }
 
   return is_valid;
Index: gcc/testsuite/gcc.target/i386/pr52813.c
===================================================================
--- gcc/testsuite/gcc.target/i386/pr52813.c	(revision 267164)
+++ gcc/testsuite/gcc.target/i386/pr52813.c	(working copy)
@@ -1,9 +1,10 @@
 /* Ensure that stack pointer cannot be an asm clobber.  */
 /* { dg-do compile { target { ! ia32 } } } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O3 -fomit-frame-pointer" } */
 
 void
 test1 (void)
 {
-  asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */
+  asm volatile ("" : : : "%rsp"); /* { dg-warning "Stack Pointer register clobbered" } */
 }
+/* { dg-final { scan-assembler "(?n)pushq.*%rbp" } } */