[PATCH] enable -Winvalid-memory-order for C++ [PR99612]

[Andrew Pinski]

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