diff mbox

Fix c++/60272

Message ID 53069A91.4060007@redhat.com
State New
Headers show

Commit Message

Richard Henderson Feb. 21, 2014, 12:15 a.m. UTC 
On 02/20/2014 01:22 PM, Richard Henderson wrote:
> On 02/20/2014 12:09 PM, Jakub Jelinek wrote:
>> On Thu, Feb 20, 2014 at 11:49:30AM -0600, Richard Henderson wrote:
>>> Tested on x86_64 and i686, and manually inspecting the generated code.
>>> Any ideas how to regression test this?
>>
>> No idea about how to test this.
>>
>>> @@ -5330,14 +5330,23 @@ expand_builtin_atomic_compare_exchange (enum machine_mode mode, tree exp,
>>>    if (tree_fits_shwi_p (weak) && tree_to_shwi (weak) != 0)
>>>      is_weak = true;
>>>  
>>> +  if (target == const0_rtx)
>>> +    target = NULL;
>>>    oldval = expect;
>>> -  if (!expand_atomic_compare_and_swap ((target == const0_rtx ? NULL : &target),
>>> -				       &oldval, mem, oldval, desired,
>>> +
>>> +  if (!expand_atomic_compare_and_swap (&target, &oldval, mem, oldval, desired,
>>
>> I'm wondering if this shouldn't be instead:
>>   oldval = NULL;
>>   if (!expand_atomic_compare_and_swap (&target, &oldval, mem, expected, desired,
>>   				       is_weak, success, failure))
>>
>> because otherwise expand_atomic_compare_and_swap could in theory already
>> store into expect MEM, couldn't it?  I mean, it does:
>>   /* Load expected into a register for the compare and swap.  */
>>   if (MEM_P (expected))
>>     expected = copy_to_reg (expected);
>>
>>   /* Make sure we always have some place to put the return oldval.
>>      Further, make sure that place is distinct from the input expected,
>>      just in case we need that path down below.  */
>>   if (ptarget_oval == NULL
>>       || (target_oval = *ptarget_oval) == NULL
>>       || reg_overlap_mentioned_p (expected, target_oval))
>>     target_oval = gen_reg_rtx (mode);
>> so with NULL *ptarget_oval it will surely allocate a pseudo, but if it is
>> the expected MEM, as expected has been forced into register earlier,
>> I don't think it overlaps with that REG and thus it can be already stored
>> and have oldval == expect after the call.
> 
> I don't know any target that actually accepts a MEM for oldval, and since the
> current definition of __atomic_compare_and_swap_n takes an address for
> expected, we'll always have a MEM.  So at present we'll always allocate a new
> pseudo just as if we zero out oldval.
> 
> But, fair enough.  It does seem generally safer your way.

Like so.


r~
diff mbox

Patch

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 09fefe50..35969ad 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -5332,9 +5332,12 @@  expand_builtin_atomic_compare_exchange (enum machine_mode mode, tree exp,
 
   if (target == const0_rtx)
     target = NULL;
-  oldval = expect;
 
-  if (!expand_atomic_compare_and_swap (&target, &oldval, mem, oldval, desired,
+  /* Lest the rtl backend create a race condition with an imporoper store
+     to memory, always create a new pseudo for OLDVAL.  */
+  oldval = NULL;
+
+  if (!expand_atomic_compare_and_swap (&target, &oldval, mem, expect, desired,
 				       is_weak, success, failure))
     return NULL_RTX;