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

[Martin Sebor <msebor@gmail.com>]

On 1/5/22 1:45 AM, Martin Liška wrote:
> On 12/8/21 17:49, Martin Sebor via Gcc-patches 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.
>>
>> Martin
> 
> Hello.
> 
> I think the patch caused:
> 
> gcc/gimple-ssa-warn-access.cc:2844:30: warning: quoted ‘%s’ directive in 
> format; use ‘%qs’ instead [-Wformat-diag]
> 
> Can you please take a look?

I've fixed it in r12-6271.

Thanks
Martin