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? Thanks, Andrew Pinski > > Martin