On 1/27/22 16:47, Andrew Pinski wrote:
On Wed, Dec 8, 2021 at 8:50 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
Even with -Wno-system-headers enabled, the -Winvalid-memory-order
code tries to make sure calls to atomic functions with invalid
memory orders are diagnosed even though the C atomic functions
are defined as macros in the <stdatomic.h> system header.
The warning triggers at all optimization levels, including -O0.
Independently, the core diagnostic enhancements implemented earlier
this year (the warning group control) enable warnings for functions
defined in system headers that are inlined into user code. This
was done for similar reason as above: because it's desirable to
diagnose invalid calls made from user code to system functions
(e.g., buffer overflows, invalid or mismatched deallocations,
etc.)
However, the C macro solution interferes with the code diagnostic
changes and prevents the invalid memory model warnings from being
issued for the same problems in C++. In addition, because C++
atomics are ordinary (inline) functions that call the underlying
__atomic_xxx built-ins, the invalid memory orders can only be
detected with both inlining and constant propagation enabled.
The attached patch removes these limitations and enables
-Winvalid-memory-order to trigger even for C++ std::atomic,
(almost) just like it does in C, at all optimization levels
including -O0.
To make that possible I had to move -Winvalid-memory-order from
builtins.c to a GIMPLE pass where it can use context-sensitive
range info at -O0, instead of relying on constant propagation
(only available at -O1 and above). Although the same approach
could be used to emit better object code for C++ atomics at -O0
(i.e., use the right memory order instead of dropping to seq_cst),
this patch doesn't do that.)
In addition to enabling the warning I've also enhanced it to
include the memory models involved in the diagnosed call (both
the problem ones and the viable alternatives).
Tested on x86_64-linux.
Jonathan, I CC you for two reasons: a) because this solution
is based on your (as well as my own) preference for handling
C++ system headers, and because of our last week's discussion
of the false positives in std::string resulting from the same
choice there.
I don't anticipate this change to lead to the same fallout
because it's unlikely for GCC to synthesize invalid memory
orders out of thin air; and b) because the current solution
can only detect the problems in calls to atomic functions at
-O0 that are declared with attribute always_inline. This
includes member functions defined in the enclosing atomic
class but not namespace-scope functions. To make
the detection possible those would also have to be
always_inline. If that's a change you'd like to see I can
look into making it happen.
This causes gcc.target/aarch64/atomic-inst-cas.c testcase to fail on
aarch64-linux-gnu. We warn now even for the case where we don't have
undefined behavior.
In the sense the code checks the success and failure memory model
before calling __atomic_compare_exchange_n.
Testcase:
int test_cas_atomic_relaxed_consume_char (char* val, char* foo, char* bar) {
int model_s = 0;
int model_f = 1;
if (model_s < model_f) return 0;
if (model_f == 3 || model_f == 4) return 0;
return __atomic_compare_exchange_n (val, foo, bar, 0, model_s, model_f);
}
Do we really want to warn about this case and other cases where we
check the memory model before calling __atomic_compare_exchange_n?
That is, do we want to warn this early in the optimization pipeline
and even without flow control checks?
The warning runs very early intentionally: to detect the same
misuses in C++ as in C, without optimization. (In C++, constant
memory models become variables because the C++ atomics functions
are wrappers around the built-ins.)
In practice, I'd expect most calls to atomic functions to be made
with constant memory models, and code like in the test case above
to be uncommon, so I think the choice of warning at -O0 was
the right one.
Martin
