[PATCH] Fix c++/60272
Richard Henderson
rth@redhat.com
Thu Feb 20 19:38:00 GMT 2014
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.
r~
More information about the Gcc-patches
mailing list