RFR: 8186166: Generalize Atomic::cmpxchg with templates

[Roman Kennke]

Am 15.08.2017 um 17:33 schrieb David Holmes:
> Hi Roman,
>
> On 16/08/2017 12:22 AM, Roman Kennke wrote:
>> Am 14.08.2017 um 11:56 schrieb Kim Barrett:
>>>> On Aug 14, 2017, at 10:28 AM, Roman Kennke <rkennke at redhat.com> wrote:
>>>>
>>>> I still don't understand why we want to allow for different types for
>>>> the new and expected values and the memory location (T, D and U). We
>>>> haven't allowed for this before, why are we doing it now?
>>> Different types are only permitted in the pointer value case,
>>> e.g. cmpxchg(T*, D* volatile*, U*).
>>>
>>> The compare_value type is permitted to differ from the destination's
>>> value type only in cv-qualifiers.  Consider a member variable "var" of
>>> type int*.
>>>
>>>    const int* old_value = var;
>>>    ... compute new_value ...
>>>    cmpxchg(new_value, &var, old_value);
>>>
>>> It is perfectly reasonable and appropriate to const-qualify
>>> old_value's type in that way.  But that would require a source level
>>> cast if cmpxchg required an exact type match for *dest and
>>> compare_value.
>>>
>>> We permit the exchange_value type to be less cv-qualified than the
>>> *dest type.  We also permit derived to base conversions.  Both of
>>> those are natural and type-safe, and may eliminate the need for some
>>> source level casts.
>>>
>>>> We haven't allowed for this before, why are we doing it now?
>>> Because of the complete type-discarding done by cmpxchg_ptr, I suggest
>>> we have no clue what our current code might or might not be doing.
>>>
>> I don't see that anywhere. The only situations that I could find are a
>> few places where dest is cast to (volatile foo*), which makes me
>> question whether that dest field should be marked volatile in the first
>> place. I see no patterns as you described above involving any const
>
> I think it is the very existence of cmpxchg_ptr, to isolate the cases
> involving pointers, that is at issue. There should not be a need for
> cmpxchg_ptr - a well-typed cmpxchg should handle all type variants.
> And that requires the template mechanisms.
Oh I think you misunderstood. I was not taking issue with cmpxchg_ptr or
templates. cmpxchg_ptr will go away as I understand, and I am fine with
using templates. I do agree that using templates for cmpxchg() is the
right thing to do. I was asking why the new templated cmpxchg() needs 3
different types for new, expected and dest, whereas it should just be
one type (in my mind). Kim was saying that is only because of pointers
and not being able to match different cv qualifications. I looked
through the code and could not find one instance where const would be
involved, and I only found a few places where we used to cast to
volatile or cast away volatile, and it seems to me we should rather fix
those fields to be of the matching type (i.e. volatile foo*) instead. We
are accessing them using cmpxchg after all.

>> casts. And I am absolutely against introducing some machinery that is
>> not actually used. If it's not used, it's dead, and only increases debt
>> rather than decrease it. (Note: the situation would be different if it
>> were a public API. but it isn't)
>
> I don't see how this will be unused dead code - it will be used
> whenever pointers are used with cmpxchg. ??
I was arguing that adding code (e.g. template args and casting code)
specifically to deal with pointers and cv mismatches, where there are no
actual such mismatches (at least not involving const, and we can fix the
volatile stuff), would be dead code.

Roman