Patchwork [RFA:] fix PR55030, wrong code from __builtin_setjmp_receiver

login
register
mail settings
Submitter Hans-Peter Nilsson
Date Nov. 26, 2012, 3:30 a.m.
Message ID <alpine.BSF.2.02.1211252148150.41600@dair.pair.com>
Download mbox | patch
Permalink /patch/201623/
State New
Headers show

Comments

Hans-Peter Nilsson - Nov. 26, 2012, 3:30 a.m.
Thanks for the reviews!

On Mon, 19 Nov 2012, Eric Botcazou wrote:
> Thanks for the analysis.  I don't think that the redundancy of the clobber
> list matters here: the clobbers are added to all asm statements, volatile or
> not, so they aren't intended to make the volatile statements more precise in
> the hope of optimizing around them.

Oh, right.

> > I'm not sure what to do with this.  Try changing volatile_insn_p
> > adding a parameter to optionally allow volatile asms with
> > outputs to pass?  But then, when *should* that distinction be
> > done, to let such volatile asms be allowed to pass as
> > not-really-as-volatile-as-we-look-for?  I'd think "never" for
> > any of the the patched code, or maybe "only when looking at
> > effects on memory".
>
> Yes, weakening volatile_insn_p seems also dangerous to me, and I'd rather lean
> towards the opposite, conservative side.
>
> We apparently have a small conflict between the meaning of volatile asms with
> operands at the source level and volatile_insn_p.  However, I think that the
> latter interpretation is the correct one: if your asm statements have effects
> that can be described, then you should use output constraints instead of
> volatile; otherwise, you should use volatile and the output constraints have
> essentially no meaning.
>
> The gcc.dg/guality/pr36728-[12].c testcases use volatile asms in an artificial
> way to coax the compiler into following a certain behaviour, so I don't think
> that they should be taken into account to judge the correctness of the change.
> Therefore, I'd go ahead and apply the full patch below, possibly adjusting
> both testcases to cope with it.  Tentative (and untested) patch attached.
>
> I've also CCed Jakub though, as he might have a different opinion.

Ok, no comments, so let's go with adjusting the test-cases.
But, your patch for the test-cases doesn't work with (or
without) the patch for pr36728-2.c.  Bonus points for being very
close, though!

I don't really see *why* your patch shouldn't work.
Specifically, I don't see why the *wrong value* is displayed
instead of at least "<optimized out>" for any changed behavior,
so I entered PR55467 for the related observations.

By instead of your approach; not using x[0] after the asm, and
instead changing the asm to also write the result to an external
variable (i.e. an unknown operation writing there and x[0], with
x[0] as input), I think I mimicked the intended behavior, such
that x[0] is preserved as a variable until the line with the
asm.  At least nothing changes for -O1 besides debug info and
the write to the variable "a" in main, for an unpatched gcc
with/without patched test-case. (The patched test-cases all pass
for an otherwise unpatched gcc at r193777.)

Committed after committing the reapplied previous patch; r193802
and r193803.

> > gcc:
> > 	PR middle-end/55030
> > 	* builtins.c (expand_builtin_setjmp_receiver): Update comment
> > 	regarding purpose of blockage.
> > 	* emit-rtl.c [!HAVE_blockage] (gen_blockage): Similarly for
> > 	the head comment.
> > 	* rtlanal.c (volatile_insn_p): Ditto.
> > 	* doc/md.texi (blockage): Update similarly.  Change wording to
> > 	require one of two forms, rather than implying a wider choice.
> > 	* cse.c (cse_insn): Where checking for blocking insns, use
> > 	volatile_insn_p instead of manual check for volatile ASM.
> > 	* dse.c (scan_insn): Ditto.
> > 	* cselib.c (cselib_process_insn): Ditto.

testsuite:
	PR middle-end/55030
	* gcc.dg/guality/pr36728-1.c, gcc.dg/guality/pr36728-2.c (foo): Don't
	use volatile asms, use plain asms.   Where the output value for the
	asm is unused, write a global variable.

Patch

Index: gcc/testsuite/gcc.dg/guality/pr36728-1.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/pr36728-1.c	(revision 193776)
+++ gcc/testsuite/gcc.dg/guality/pr36728-1.c	(working copy)
@@ -1,7 +1,7 @@ 
 /* PR debug/36728 */
 /* { dg-do run } */
 /* { dg-options "-g" } */
-
+int a;
 int __attribute__((noinline))
 foo (int arg1, int arg2, int arg3, int arg4, int arg5, int arg6, int arg7)
 {
@@ -9,9 +9,9 @@  foo (int arg1, int arg2, int arg3, int a
   int __attribute__ ((aligned(32))) y;

   y = 2;
-  asm volatile ("" : "=m" (y) : "m" (y));
+  asm ("" : "=m" (y) : "m" (y));
   x[0] = 25;
-  asm volatile ("" : "=m" (x[0]) : "m" (x[0]));
+  asm ("" : "=m" (x[0]), "=m" (a) : "m" (x[0]));
   return y;
 }

@@ -43,7 +43,7 @@  int
 main ()
 {
   int l = 0;
-  asm volatile ("" : "=r" (l) : "0" (l));
-  foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30);
+  asm ("" : "=r" (l) : "0" (l));
+  a = foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30);
   return 0;
 }
Index: gcc/testsuite/gcc.dg/guality/pr36728-2.c
===================================================================
--- gcc/testsuite/gcc.dg/guality/pr36728-2.c	(revision 193776)
+++ gcc/testsuite/gcc.dg/guality/pr36728-2.c	(working copy)
@@ -1,7 +1,7 @@ 
 /* PR debug/36728 */
 /* { dg-do run } */
 /* { dg-options "-g" } */
-
+int a;
 int __attribute__((noinline))
 foo (int arg1, int arg2, int arg3, int arg4, int arg5, int arg6, int arg7)
 {
@@ -9,9 +9,9 @@  foo (int arg1, int arg2, int arg3, int a
   int __attribute__ ((aligned(32))) y;

   y = 2;
-  asm volatile ("" : "=m" (y) : "m" (y));
+  asm ("" : "=m" (y) : "m" (y));
   x[0] = 25;
-  asm volatile ("" : "=m" (x[0]) : "m" (x[0]));
+  asm ("" : "=m" (x[0]), "=m" (a) : "m" (x[0]));
   return y;
 }

@@ -43,7 +43,7 @@  int
 main ()
 {
   int l = 0;
-  asm volatile ("" : "=r" (l) : "0" (l));
-  foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30);
+  asm ("" : "=r" (l) : "0" (l));
+  a = foo (l + 1, l + 2, l + 3, l + 4, l + 5, l + 6, l + 30);
   return 0;
 }