[RFC] Should an illegal C++11 memory model in an atomic op be an error?

Hi,

Using the builtin functions for atomic operations that honor the C++11 memory model requirements will result in undefined behavior if an illegal memory model is specified. Consequently, we can optimize-away code that uses the result of one of these incorrectly invoked atomic ops.

For example, for the function:

type __atomic_load_n (type *ptr, int memmodel)

the models __ATOMIC_RELEASE and __ATOMIC_ACQ_REL are not legal. So in a test-case like:

int test_good(int* p_atomic, int* p1, int* p2) {

// __ATOMIC_RELAXED memmodel is legal for __atomic_load_n()

int v1 = *p1;

int atomic_val = __atomic_load_n(p_atomic, __ATOMIC_RELAXED);

int v2 = *p2;

return v1 + v2 + atomic_val;

}

int test_bad(int* p_atomic, int* p1, int* p2) {

// __ATOMIC_RELEASE memmodel is illegal for __atomic_load_n()

int v1 = *p1;

int atomic_val = __atomic_load_n(p_atomic, __ATOMIC_RELEASE);

int v2 = *p2;

return v1 + v2 + atomic_val;

}

since the ‘test_bad()’ function is using an illegal memmodel parameter, the behavior is undefined. Given that that function uses the result (‘atomic_val’) of that illegal atomic op in the return expression, we notice that the return-value of the function is undefined, and therefore at -O2, we legitimately optimize-away anything that feeds into computing that return value.

Prior to Clang 3.5, this illegal memmodel was accepted without a diagnostic (even with -Weverything). Beginning with 3.5, we produce:

warning: memory order argument to atomic operation is invalid

So it’s now better for users. But since we freely optimize-away computations that are based on the result of one of these illegal memmodel uses, it seems like it would be reasonable to treat it as an error, rather than a warning.

What do people think?

FTR, trying this with g++ (version 4.8.2), shows that GNU considers it an error:

error: invalid memory model for `__atomic_load’

Thanks,

-Warren

Looks like it was deliberately demoted to a warning:

Sean:~/pg/llvm/tools/clang/include/clang/Basic % git log -Swarn_atomic_op_has_invalid_memory_order

commit 7533ae94377aed5aa7866ebd67cbcf616efabb3c

Author: Tim Northover <tnorthover@apple.com>

Sema: demote invalid atomic ordering message to warning.

Someone could write:

if (0) {

__c11_atomic_load(ptr, memory_order_release);

}

or the equivalent, which is perfectly valid, so we shouldn’t outright reject

invalid orderings on purely static grounds.

rdar://problem/16242991

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@203564 91177308-0d34-0410-b5e6-96231b3b80d8

Tim, could you shed a bit more light on the context of this patch in rdar://problem/16242991 ?

– Sean Silva

Hi Sean,

Tim, could you shed a bit more light on the context of this patch in rdar://problem/16242991 ?

There's not really any more context: the original bug was asking for some diagnostic, I decided it had to be a warning because legitimate C code can (statically) contain calls like that. It's most likely to come up with macro or template expansion of course.

Cheers.

Tim.

Hi Sean,

> Tim, could you shed a bit more light on the context of this patch in
rdar://problem/16242991 ?

There's not really any more context: the original bug was asking for some
diagnostic, I decided it had to be a warning because legitimate C code can
(statically) contain calls like that. It's most likely to come up with
macro or template expansion of course.

Makes sense.

-- Sean Silva

I'd be in favor or promoting it to an error. We recently started rejecting
this code, for example:

void f(int n) {
  if (__builtin_constant_p(n))
    __asm__("" : : "I"(n));
  else
    __asm__("" : : "m"(n));
}

I'd be in favor or promoting it to an error. We recently started rejecting this code, for example:

I'm not sure I would. Deciding on the diagnostics for our own intrinsics is one thing, but this would trigger on well-formed programs according to the standard.

That's rather a bigger step to take, and we should probably be comparing it to situations with that similarity (the only one I know of is ARM64 complaining more strongly about functions without prototypes).

Cheers.

Tim.

> I'd be in favor or promoting it to an error. We recently started
rejecting this code, for example:

I'm not sure I would. Deciding on the diagnostics for our own intrinsics
is one thing, but this would trigger on well-formed programs according to
the standard.

I agree that this should not be an error. The builtin does not require a
constant; we shouldn't say it's ill-formed if we happen to be able to
evaluate the order parameter and find it's out-of-range.

I think this sort of thing is completely reasonable (perhaps in the
implementation of a C++11-compatible atomics library):

#define atomic_builtin(kind, order, ...) \
  ({ switch(order) { \
    case memory_order_relaxed: kind(__VA_ARGS__, __ATOMIC_RELAXED); break; \
    [...]

That's rather a bigger step to take, and we should probably be comparing it

I’m convinced that it’s best to keep it as a warning.

In reading over the responses, one thing that comes to mind which I think is analogous is

a shift by a literal amount that’s too large. For example, a shift of a 4-byte ‘int’ by

the literal 32 results in a warning, not an error:

unsigned int foo(unsigned int a0, unsigned int a1)

{

return (a0 >> 32) + a1;

}

gets:

warning: shift count >= width of type [-Wshift-count-overflow]

So for this, we give a warning. And at -O2, we legitimately optimize-away all expressions

based on the result of the undefined-behavior-shift (producing just a ‘return’ in the

above case). I wouldn’t consider changing the warning that comes for the above shift to

an error, and so by the same token, I no longer consider an incorrect (literal)

memory-model parameter for an atomic op to be a good candidate for an error. Keeping it

a warning keeps us consistent in this regard.

Thanks all, for the feedback.

-Warren