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

[Jeff Law <jeffreyalaw@gmail.com>]

On 12/8/2021 9:49 AM, 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
>
> gcc-99612.diff
>
> PR middle-end/99612 - Remove "#pragma GCC system_header" from atomic file to warn on incorrect memory order
>
> gcc/ChangeLog:
>
> 	PR middle-end/99612
> 	* builtins.c (get_memmodel): Move warning code to
> 	gimple-ssa-warn-access.cc.
> 	(expand_builtin_atomic_compare_exchange): Same.
> 	(expand_ifn_atomic_compare_exchange): Same.
> 	(expand_builtin_atomic_load): Same.
> 	(expand_builtin_atomic_store): Same.
> 	(expand_builtin_atomic_clear): Same.
> 	* doc/extend.texi (__atomic_exchange_n): Update valid memory
> 	models.
> 	* gimple-ssa-warn-access.cc (memmodel_to_uhwi): New function.
> 	(struct memmodel_pair): New struct.
> 	(memmodel_name): New function.
> 	(pass_waccess::maybe_warn_memmodel): New function.
> 	(pass_waccess::check_atomic_memmodel): New function.
> 	(pass_waccess::check_atomic_builtin): Handle memory model.
> 	* input.c (expansion_point_location_if_in_system_header): Return
> 	original location if expansion location is in a system header.
>
> gcc/testsuite/ChangeLog:
>
> 	PR middle-end/99612
> 	* c-c++-common/pr83059.c: Adjust text of expected diagnostics.
> 	* gcc.dg/atomic-invalid-2.c: Same.
> 	* gcc.dg/atomic-invalid.c: Same.
> 	* c-c++-common/Winvalid-memory-model.c: New test.
> 	* g++.dg/warn/Winvalid-memory-model-2.C: New test.
> 	* g++.dg/warn/Winvalid-memory-model.C: New test.
Probably larger than I would have liked for a stage3 submitted bugfix.  
But it looks reasonable and as you mentioned, I think the potential for 
fallout is relatively small.

OK.

jeff