Patchwork [RFA:] fix PR54261, reverse operator emitted for compare_and_swap-libfunc targets

login
register
mail settings
Submitter Hans-Peter Nilsson
Date Aug. 15, 2012, 12:20 a.m.
Message ID <201208150020.q7F0KbaV006568@ignucius.se.axis.com>
Download mbox | patch
Permalink /patch/177522/
State New
Headers show

Comments

Hans-Peter Nilsson - Aug. 15, 2012, 12:20 a.m.
If a target implements (some) atomics by only setting
sync_compare_and_swap_optab libfuncs (see
cris.c:cris_init_libfuncs), a code-path less travelled is used.
There's a bug there, causing sync/atomic operators to be
implemented with the reverse operator, e.g. minus instead of
plus.

This should have been trivially caught by the test-suite, but
wasn't, for three reasons.
 One, the sync_* test-suite effective-targets use hard-coded
target tuples, and I haven't added cris and crisv32 there, doh
(I see also m68k missing).  I'll do that, with separate testing,
perhaps later try to make the test general.
 Two, PR54266: __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 and friends, as
used in the test-suite effective-target cas_int, is of no use
for targets implementing (some) atomics only through library
functions.
 Three, mea culpa; I missed the following libstdc++ test-suite
failures (4.7):
FAIL: 20_util/shared_ptr/thread/default_weaktoshared.cc execution test
FAIL: 20_util/shared_ptr/thread/mutex_weaktoshared.cc execution test
FAIL: 21_strings/basic_string/pthread4.cc execution test
FAIL: 23_containers/map/pthread6.cc execution test
FAIL: 27_io/basic_ofstream/pthread2.cc execution test
FAIL: 27_io/basic_ostringstream/pthread3.cc execution test
FAIL: 30_threads/thread/members/4.cc execution test
FAIL: 30_threads/thread/members/5.cc execution test
FAIL: 30_threads/unique_lock/locking/2.cc execution test
FAIL: tr1/2_general_utilities/shared_ptr/thread/default_weaktoshared.cc execution test
FAIL: tr1/2_general_utilities/shared_ptr/thread/mutex_weaktoshared.cc execution test

There *were* also some failures in the core gcc test-suite, like:
FAIL: gcc.target/cris/torture/sync-mis-op-i-1ml.c  -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  (test for excess errors)
with gcc.log having:
ccDrw3V5.ltrans0.o:(.text.startup+0x2a): undefined reference to `__atomic_fetch_nand_4'
but that didn't make sense to me so I wrote it off as LTO
weirdness (something wrong in LTO handling sync libfuncs with
4.7).  It still doesn't really make sense to me, i.e. that the
FAILs are now gone.

I looked around and it seems only cris{v32,}-axis-linux-gnu is
affected.  Still, besides that target, for a 4.7/r189762 import
and c/c++ testing, boot+regtest in progress for x86_64-linux and
cross-test for cris-axis-elf.  The test-case is spot-checked to
pass for a target not implementing any atomics whatsoever,
i.e. mmix-knuth-mmixware.

Ok for trunk, assuming clean test-results?  Maybe 4.7 too, it
being somewhat trivial?

gcc:
	PR middle-end/54261
	* optabs.c (expand_atomic_fetch_op): Save and restore code when
	retrying after failed attempt.

gcc/testsuite:
	PR middle-end/54261
	* gcc.dg/torture/pr54261-1.c: New test.


brgds, H-P

Patch

--- /dev/null
+++ gcc/testsuite/gcc.dg/torture/pr54261-1.c
@@ -0,0 +1,42 @@ 
+/* { dg-do run } */
+/* { dg-additional-options "-DSYNC_FALLBACK" { target { ! cas_int } } } */
+
+#ifdef SYNC_FALLBACK
+/* The SYNC_FALLBACK code is just so we don't have to restrict this test
+   to any subset of targets.  For targets with no atomics support at
+   all, the cas_int effective-target is false and the fallback provides
+   a PASS.  Where the bug trigs (at the time this test-case was added),
+   cas_int is also false but the fallback isn't used.  */
+__attribute__((__noinline__, __noclone__))
+int
+# if __INT_MAX__ == 0x7fff
+ __sync_fetch_and_add_2
+# else
+ __sync_fetch_and_add_4
+# endif
+ (int *at, int val)
+{
+  int tmp = *at;
+  asm ("");
+  *at = tmp + val;
+  return tmp;
+}
+#endif
+
+__attribute__((__noinline__, __noclone__))
+void g (int *at, int val)
+{
+  asm ("");
+  __sync_fetch_and_add (at, val);
+}
+
+int main(void)
+{
+  int x = 41;
+  int a = 1;
+  g (&x, a);
+
+  if (x != 42)
+    __builtin_abort ();
+  __builtin_exit (0);
+}

(svn diff -x -U11 optabs.c used to show "code" being changed and
then the conditional failure; nowhere else in
expand_atomic_fetch_op is that done unless succeeded by
unconditional success and return)

Index: optabs.c
===================================================================
--- gcc/optabs.c	(revision 190398)
+++ gcc/optabs.c	(working copy)
@@ -7818,22 +7818,23 @@  expand_atomic_fetch_op (rtx target, rtx 
 	    result = expand_simple_binop (mode, code, result, val, target,
 					  true, OPTAB_LIB_WIDEN);
 	  return result;
 	}
     }
 
   /* Try the __sync libcalls only if we can't do compare-and-swap inline.  */
   if (!can_compare_and_swap_p (mode, false))
     {
       rtx libfunc;
       bool fixup = false;
+      enum rtx_code orig_code = code;
 
       libfunc = optab_libfunc (after ? optab.fetch_after
 			       : optab.fetch_before, mode);
       if (libfunc == NULL
 	  && (after || unused_result || optab.reverse_code != UNKNOWN))
 	{
 	  fixup = true;
 	  if (!after)
 	    code = optab.reverse_code;
 	  libfunc = optab_libfunc (after ? optab.fetch_before
 				   : optab.fetch_after, mode);
@@ -7841,22 +7842,25 @@  expand_atomic_fetch_op (rtx target, rtx 
       if (libfunc != NULL)
 	{
 	  rtx addr = convert_memory_address (ptr_mode, XEXP (mem, 0));
 	  result = emit_library_call_value (libfunc, NULL, LCT_NORMAL, mode,
 					    2, addr, ptr_mode, val, mode);
 
 	  if (!unused_result && fixup)
 	    result = expand_simple_binop (mode, code, result, val, target,
 					  true, OPTAB_LIB_WIDEN);
 	  return result;
 	}
+
+      /* We need the original code for any further attempts.  */
+      code = orig_code;
     }
 
   /* If nothing else has succeeded, default to a compare and swap loop.  */
   if (can_compare_and_swap_p (mode, true))
     {
       rtx insn;
       rtx t0 = gen_reg_rtx (mode), t1;
 
       start_sequence ();
 
       /* If the result is used, get a register for it.  */