diff mbox series

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

Message ID efdaf72b-15b8-84b8-b174-fd4d25c7de31@gmail.com
State New
Headers show
Series enable -Winvalid-memory-order for C++ [PR99612] | expand

Commit Message

Martin Sebor Dec. 8, 2021, 4:49 p.m. UTC
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

Comments

Jonathan Wakely Dec. 8, 2021, 5:14 p.m. UTC | #1
On Wed, 8 Dec 2021 at 16:49, Martin Sebor wrote:
> 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;

Agreed. I don't think we'll have the same kind of issues. 99% of uses
of memory orders just use the constants explicitly, passing them
directly to the std::atomic member functions (or something that calls
them).

>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.

I think we can ignore the namespace-scope functions in <atomic>. Most people do.
Martin Sebor Dec. 8, 2021, 6:12 p.m. UTC | #2
On 12/8/21 10:14 AM, Jonathan Wakely wrote:
> On Wed, 8 Dec 2021 at 16:49, Martin Sebor wrote:
>> 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;
> 
> Agreed. I don't think we'll have the same kind of issues. 99% of uses
> of memory orders just use the constants explicitly, passing them
> directly to the std::atomic member functions (or something that calls
> them).

Ack.

> 
>> 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.
> 
> I think we can ignore the namespace-scope functions in <atomic>. Most people do.

I was thinking it would be nice to provide the same quality
for the C/C++ portability interface (see the test below that
triggers the warning at -O0 in C but requires -O1 to get it
in C++).  But it's not a big deal.

Martin

#if __cplusplus
#  include <atomic>
using std::memory_order_release;
using std::atomic_load_explicit;
extern std::atomic<int> eai;
#else
#  include <stdatomic.h>
extern _Atomic int eai;
#endif

int load (void)
{
   return atomic_load_explicit (&eai, memory_order_release);
}
Martin Sebor Dec. 15, 2021, 3:30 p.m. UTC | #3
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586402.html

Besides PR 99612 this also fixes the false positive reported
recently in PR 103372.

On 12/8/21 9:49 AM, Martin Sebor 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
Jeff Law Dec. 23, 2021, 11:20 p.m. UTC | #4
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
Martin Liška Jan. 5, 2022, 8:45 a.m. UTC | #5
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?

Thanks,
Martin
Martin Sebor Jan. 5, 2022, 8:34 p.m. UTC | #6
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
Andrew Pinski Jan. 27, 2022, 11:47 p.m. UTC | #7
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
Andrew Pinski Jan. 27, 2022, 11:48 p.m. UTC | #8
On Thu, Jan 27, 2022 at 3:47 PM Andrew Pinski <pinskia@gmail.com> 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?

I should mention I filed this as PR 104200.

Thanks,
Andrew

>
> Thanks,
> Andrew Pinski
>
> >
> > Martin
Martin Sebor Jan. 28, 2022, 12:59 a.m. UTC | #9
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
Jonathan Wakely Jan. 28, 2022, 11:05 a.m. UTC | #10
On Fri, 28 Jan 2022 at 00:59, Martin Sebor wrote:
> 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.

Some of us consider it a misfeature that the C++ functions use
function parameters for the memory model at all. They could have been
template arguments, so that only constants would be allowed:

atomic.fetch<memory_order_seq_cst>();
diff mbox series

Patch

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.


diff --git a/gcc/builtins.c b/gcc/builtins.c
index 03829c03a5a..e7d0c276751 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -5790,35 +5790,22 @@  expand_builtin_sync_lock_release (machine_mode mode, tree exp)
 static enum memmodel
 get_memmodel (tree exp)
 {
-  rtx op;
-  unsigned HOST_WIDE_INT val;
-  location_t loc
-    = expansion_point_location_if_in_system_header (input_location);
-
   /* If the parameter is not a constant, it's a run time value so we'll just
      convert it to MEMMODEL_SEQ_CST to avoid annoying runtime checking.  */
   if (TREE_CODE (exp) != INTEGER_CST)
     return MEMMODEL_SEQ_CST;
 
-  op = expand_normal (exp);
+  rtx op = expand_normal (exp);
 
-  val = INTVAL (op);
+  unsigned HOST_WIDE_INT val = INTVAL (op);
   if (targetm.memmodel_check)
     val = targetm.memmodel_check (val);
   else if (val & ~MEMMODEL_MASK)
-    {
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "unknown architecture specifier in memory model to builtin");
-      return MEMMODEL_SEQ_CST;
-    }
+    return MEMMODEL_SEQ_CST;
 
   /* Should never see a user explicit SYNC memodel model, so >= LAST works. */
   if (memmodel_base (val) >= MEMMODEL_LAST)
-    {
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "invalid memory model argument to builtin");
-      return MEMMODEL_SEQ_CST;
-    }
+    return MEMMODEL_SEQ_CST;
 
   /* Workaround for Bugzilla 59448. GCC doesn't track consume properly, so
      be conservative and promote consume to acquire.  */
@@ -5865,28 +5852,17 @@  expand_builtin_atomic_compare_exchange (machine_mode mode, tree exp,
 {
   rtx expect, desired, mem, oldval;
   rtx_code_label *label;
-  enum memmodel success, failure;
   tree weak;
   bool is_weak;
-  location_t loc
-    = expansion_point_location_if_in_system_header (input_location);
 
-  success = get_memmodel (CALL_EXPR_ARG (exp, 4));
-  failure = get_memmodel (CALL_EXPR_ARG (exp, 5));
+  memmodel success = get_memmodel (CALL_EXPR_ARG (exp, 4));
+  memmodel failure = get_memmodel (CALL_EXPR_ARG (exp, 5));
 
   if (failure > success)
-    {
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "failure memory model cannot be stronger than success "
-		  "memory model for %<__atomic_compare_exchange%>");
-      success = MEMMODEL_SEQ_CST;
-    }
+    success = MEMMODEL_SEQ_CST;
  
   if (is_mm_release (failure) || is_mm_acq_rel (failure))
     {
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "invalid failure memory model for "
-		  "%<__atomic_compare_exchange%>");
       failure = MEMMODEL_SEQ_CST;
       success = MEMMODEL_SEQ_CST;
     }
@@ -5991,29 +5967,15 @@  expand_ifn_atomic_compare_exchange (gcall *call)
   int size = tree_to_shwi (gimple_call_arg (call, 3)) & 255;
   gcc_assert (size == 1 || size == 2 || size == 4 || size == 8 || size == 16);
   machine_mode mode = int_mode_for_size (BITS_PER_UNIT * size, 0).require ();
-  rtx expect, desired, mem, oldval, boolret;
-  enum memmodel success, failure;
-  tree lhs;
-  bool is_weak;
-  location_t loc
-    = expansion_point_location_if_in_system_header (gimple_location (call));
 
-  success = get_memmodel (gimple_call_arg (call, 4));
-  failure = get_memmodel (gimple_call_arg (call, 5));
+  memmodel success = get_memmodel (gimple_call_arg (call, 4));
+  memmodel failure = get_memmodel (gimple_call_arg (call, 5));
 
   if (failure > success)
-    {
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "failure memory model cannot be stronger than success "
-		  "memory model for %<__atomic_compare_exchange%>");
-      success = MEMMODEL_SEQ_CST;
-    }
+    success = MEMMODEL_SEQ_CST;
 
   if (is_mm_release (failure) || is_mm_acq_rel (failure))
     {
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "invalid failure memory model for "
-		  "%<__atomic_compare_exchange%>");
       failure = MEMMODEL_SEQ_CST;
       success = MEMMODEL_SEQ_CST;
     }
@@ -6025,15 +5987,15 @@  expand_ifn_atomic_compare_exchange (gcall *call)
     }
 
   /* Expand the operands.  */
-  mem = get_builtin_sync_mem (gimple_call_arg (call, 0), mode);
+  rtx mem = get_builtin_sync_mem (gimple_call_arg (call, 0), mode);
 
-  expect = expand_expr_force_mode (gimple_call_arg (call, 1), mode);
-  desired = expand_expr_force_mode (gimple_call_arg (call, 2), mode);
+  rtx expect = expand_expr_force_mode (gimple_call_arg (call, 1), mode);
+  rtx desired = expand_expr_force_mode (gimple_call_arg (call, 2), mode);
 
-  is_weak = (tree_to_shwi (gimple_call_arg (call, 3)) & 256) != 0;
+  bool is_weak = (tree_to_shwi (gimple_call_arg (call, 3)) & 256) != 0;
 
-  boolret = NULL;
-  oldval = NULL;
+  rtx boolret = NULL;
+  rtx oldval = NULL;
 
   if (!expand_atomic_compare_and_swap (&boolret, &oldval, mem, expect, desired,
 				       is_weak, success, failure))
@@ -6042,7 +6004,7 @@  expand_ifn_atomic_compare_exchange (gcall *call)
       return;
     }
 
-  lhs = gimple_call_lhs (call);
+  tree lhs = gimple_call_lhs (call);
   if (lhs)
     {
       rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE);
@@ -6061,24 +6023,15 @@  expand_ifn_atomic_compare_exchange (gcall *call)
 static rtx
 expand_builtin_atomic_load (machine_mode mode, tree exp, rtx target)
 {
-  rtx mem;
-  enum memmodel model;
-
-  model = get_memmodel (CALL_EXPR_ARG (exp, 1));
+  memmodel model = get_memmodel (CALL_EXPR_ARG (exp, 1));
   if (is_mm_release (model) || is_mm_acq_rel (model))
-    {
-      location_t loc
-	= expansion_point_location_if_in_system_header (input_location);
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "invalid memory model for %<__atomic_load%>");
-      model = MEMMODEL_SEQ_CST;
-    }
+    model = MEMMODEL_SEQ_CST;
 
   if (!flag_inline_atomics)
     return NULL_RTX;
 
   /* Expand the operand.  */
-  mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
+  rtx mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
 
   return expand_atomic_load (target, mem, model);
 }
@@ -6092,26 +6045,17 @@  expand_builtin_atomic_load (machine_mode mode, tree exp, rtx target)
 static rtx
 expand_builtin_atomic_store (machine_mode mode, tree exp)
 {
-  rtx mem, val;
-  enum memmodel model;
-
-  model = get_memmodel (CALL_EXPR_ARG (exp, 2));
+  memmodel model = get_memmodel (CALL_EXPR_ARG (exp, 2));
   if (!(is_mm_relaxed (model) || is_mm_seq_cst (model)
 	|| is_mm_release (model)))
-    {
-      location_t loc
-	= expansion_point_location_if_in_system_header (input_location);
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "invalid memory model for %<__atomic_store%>");
-      model = MEMMODEL_SEQ_CST;
-    }
+    model = MEMMODEL_SEQ_CST;
 
   if (!flag_inline_atomics)
     return NULL_RTX;
 
   /* Expand the operands.  */
-  mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
-  val = expand_expr_force_mode (CALL_EXPR_ARG (exp, 1), mode);
+  rtx mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
+  rtx val = expand_expr_force_mode (CALL_EXPR_ARG (exp, 1), mode);
 
   return expand_atomic_store (mem, val, model, false);
 }
@@ -6282,29 +6226,19 @@  expand_ifn_atomic_bit_test_and (gcall *call)
 static rtx
 expand_builtin_atomic_clear (tree exp) 
 {
-  machine_mode mode;
-  rtx mem, ret;
-  enum memmodel model;
-
-  mode = int_mode_for_size (BOOL_TYPE_SIZE, 0).require ();
-  mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
-  model = get_memmodel (CALL_EXPR_ARG (exp, 1));
+  machine_mode mode = int_mode_for_size (BOOL_TYPE_SIZE, 0).require ();
+  rtx mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
+  memmodel model = get_memmodel (CALL_EXPR_ARG (exp, 1));
 
   if (is_mm_consume (model) || is_mm_acquire (model) || is_mm_acq_rel (model))
-    {
-      location_t loc
-	= expansion_point_location_if_in_system_header (input_location);
-      warning_at (loc, OPT_Winvalid_memory_model,
-		  "invalid memory model for %<__atomic_store%>");
-      model = MEMMODEL_SEQ_CST;
-    }
+    model = MEMMODEL_SEQ_CST;
 
   /* Try issuing an __atomic_store, and allow fallback to __sync_lock_release.
      Failing that, a store is issued by __atomic_store.  The only way this can
      fail is if the bool type is larger than a word size.  Unlikely, but
      handle it anyway for completeness.  Assume a single threaded model since
      there is no atomic support in this case, and no barriers are required.  */
-  ret = expand_atomic_store (mem, const0_rtx, model, true);
+  rtx ret = expand_atomic_store (mem, const0_rtx, model, true);
   if (!ret)
     emit_move_insn (mem, const0_rtx);
   return const0_rtx;
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 48bf8aaff50..681c9f6a29c 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -29,6 +29,7 @@ 
 #include "gimple.h"
 #include "tree-pass.h"
 #include "builtins.h"
+#include "diagnostic.h"
 #include "ssa.h"
 #include "gimple-pretty-print.h"
 #include "gimple-ssa-warn-access.h"
@@ -38,6 +39,8 @@ 
 #include "gimple-fold.h"
 #include "gimple-iterator.h"
 #include "langhooks.h"
+#include "memmodel.h"
+#include "target.h"
 #include "tree-dfa.h"
 #include "tree-ssa.h"
 #include "tree-cfg.h"
@@ -2103,6 +2106,8 @@  private:
 
   void maybe_check_dealloc_call (gcall *);
   void maybe_check_access_sizes (rdwr_map *, tree, tree, gimple *);
+  bool maybe_warn_memmodel (gimple *, tree, tree, const unsigned char *);
+  void check_atomic_memmodel (gimple *, tree, tree, const unsigned char *);
 
   /* A pointer_query object and its cache to store information about
      pointers and their targets in.  */
@@ -2686,6 +2691,237 @@  pass_waccess::check_read_access (gimple *stmt, tree src,
 		&data, m_ptr_qry.rvals);
 }
 
+/* Return true if memory model ORD is constant in the context of STMT and
+   set *CSTVAL to the constant value.  Otherwise return false.  Warn for
+   invalid ORD.  */
+
+bool
+memmodel_to_uhwi (tree ord, gimple *stmt, unsigned HOST_WIDE_INT *cstval)
+{
+  unsigned HOST_WIDE_INT val;
+
+  if (TREE_CODE (ord) == INTEGER_CST)
+    {
+      if (!tree_fits_uhwi_p (ord))
+	return false;
+      val = tree_to_uhwi (ord);
+    }
+  else
+    {
+      /* Use the range query to determine constant values in the absence
+	 of constant proppagation (such as at -O0).  */
+      value_range rng;
+      if (!get_range_query (cfun)->range_of_expr (rng, ord, stmt)
+	  || !rng.constant_p ()
+	  || !rng.singleton_p (&ord))
+	return false;
+
+      wide_int lob = rng.lower_bound ();
+      if (!wi::fits_uhwi_p (lob))
+	return false;
+
+      val = lob.to_shwi ();
+    }
+
+  if (targetm.memmodel_check)
+    /* This might warn for an invalid VAL but return a conservatively
+       valid result.  */
+    val = targetm.memmodel_check (val);
+  else if (val & ~MEMMODEL_MASK)
+    {
+      tree fndecl = gimple_call_fndecl (stmt);
+      location_t loc = gimple_location (stmt);
+      loc = expansion_point_location_if_in_system_header (loc);
+
+      warning_at (loc, OPT_Winvalid_memory_model,
+		  "unknown architecture specifier in memory model "
+		  "%wi for %qD", val, fndecl);
+      return false;
+    }
+
+  *cstval = val;
+
+  return true;
+}
+
+/* Valid memory model for each set of atomic built-in functions.  */
+
+struct memmodel_pair
+{
+  memmodel modval;
+  const char* modname;
+
+#define MEMMODEL_PAIR(val, str)			\
+  { MEMMODEL_ ## val, "memory_order_" str }
+};
+
+/* Valid memory models in the order of increasing strength.  */
+
+static const memmodel_pair memory_models[] =
+  { MEMMODEL_PAIR (RELAXED, "relaxed"),
+    MEMMODEL_PAIR (SEQ_CST, "seq_cst"),
+    MEMMODEL_PAIR (ACQUIRE, "acquire"),
+    MEMMODEL_PAIR (CONSUME, "consume"),
+    MEMMODEL_PAIR (RELEASE, "release"),
+    MEMMODEL_PAIR (ACQ_REL, "acq_rel")
+  };
+
+/* Return the name of the memory model VAL.  */
+
+static const char*
+memmodel_name (unsigned HOST_WIDE_INT val)
+{
+  val = memmodel_base (val);
+
+  for (unsigned i = 0; i != sizeof memory_models / sizeof *memory_models; ++i)
+    {
+      if (val == memory_models[i].modval)
+	return memory_models[i].modname;
+    }
+  return NULL;
+}
+
+/* Indices of valid MEMORY_MODELS above for corresponding atomic operations.  */
+static const unsigned char load_models[] = { 0, 1, 2, 3, UCHAR_MAX };
+static const unsigned char store_models[] = { 0, 1, 4, UCHAR_MAX };
+static const unsigned char xchg_models[] = { 0, 1, 3, 4, 5, UCHAR_MAX };
+static const unsigned char flag_clr_models[] = { 0, 1, 4, UCHAR_MAX };
+static const unsigned char all_models[] = { 0, 1, 2, 3, 4, 5, UCHAR_MAX };
+
+/* Check the success memory model argument ORD_SUCS to the call STMT to
+   an atomic function and warn if it's invalid.  If nonnull, also check
+   the failure memory model ORD_FAIL and warn if it's invalid.  Return
+   true if a warning has been issued.  */
+
+bool
+pass_waccess::maybe_warn_memmodel (gimple *stmt, tree ord_sucs,
+				   tree ord_fail, const unsigned char *valid)
+{
+  unsigned HOST_WIDE_INT sucs, fail = 0;
+  if (!memmodel_to_uhwi (ord_sucs, stmt, &sucs)
+      || (ord_fail && !memmodel_to_uhwi (ord_fail, stmt, &fail)))
+    return false;
+
+  bool is_valid = false;
+  if (valid)
+    for (unsigned i = 0; valid[i] != UCHAR_MAX; ++i)
+      {
+	memmodel model = memory_models[valid[i]].modval;
+	if (memmodel_base (sucs) == model)
+	  {
+	    is_valid = true;
+	    break;
+	  }
+      }
+  else
+    is_valid = true;
+
+  tree fndecl = gimple_call_fndecl (stmt);
+  location_t loc = gimple_location (stmt);
+  loc = expansion_point_location_if_in_system_header (loc);
+
+  if (!is_valid)
+    {
+      bool warned = false;
+      if (const char *modname = memmodel_name (sucs))
+	warned = warning_at (loc, OPT_Winvalid_memory_model,
+			     "invalid memory model %qs for %qD",
+			     modname, fndecl);
+      else
+	warned = warning_at (loc, OPT_Winvalid_memory_model,
+			     "invalid memory model %wi for %qD",
+			     sucs, fndecl);
+
+      if (!warned)
+	return false;
+
+      /* Print a note with the valid memory models.  */
+      pretty_printer pp;
+      pp_show_color (&pp) = pp_show_color (global_dc->printer);
+      for (unsigned i = 0; valid[i] != UCHAR_MAX; ++i)
+	{
+	  const char *modname = memory_models[valid[i]].modname;
+	  pp_printf (&pp, "%s%<%s%>", i ? ", " : "", modname);
+	}
+
+      inform (loc, "valid models are %s", pp_formatted_text (&pp));
+      return true;
+    }
+
+  if (!ord_fail)
+    return false;
+
+  if (fail == MEMMODEL_RELEASE || fail == MEMMODEL_ACQ_REL)
+    if (const char *failname = memmodel_name (fail))
+      {
+	/* If both memory model arguments are valid but their combination
+	   is not, use their names in the warning.  */
+	if (!warning_at (loc, OPT_Winvalid_memory_model,
+			 "invalid failure memory model %qs for %qD",
+			 failname, fndecl))
+	  return false;
+
+	inform (loc,
+		"valid failure models are %qs, %qs, %qs, %qs",
+		"memory_order_relaxed", "memory_order_seq_cst",
+		"memory_order_acquire", "memory_order_consume");
+	return true;
+      }
+
+  if (memmodel_base (fail) <= memmodel_base (sucs))
+    return false;
+
+  if (const char *sucsname = memmodel_name (sucs))
+    if (const char *failname = memmodel_name (fail))
+      {
+	/* If both memory model arguments are valid but their combination
+	   is not, use their names in the warning.  */
+	if (!warning_at (loc, OPT_Winvalid_memory_model,
+			 "failure memory model %qs cannot be stronger "
+			 "than success memory model %qs for %qD",
+			 failname, sucsname, fndecl))
+	  return false;
+
+	/* Print a note with the valid failure memory models which are
+	   those with a value less than or equal to the success mode.  */
+	char buf[120];
+	*buf = '\0';
+	for (unsigned i = 0;
+	     memory_models[i].modval <= memmodel_base (sucs); ++i)
+	  {
+	    if (*buf)
+	      strcat (buf, ", ");
+
+	    const char *modname = memory_models[valid[i]].modname;
+	    sprintf (buf + strlen (buf), "'%s'", modname);
+	  }
+
+	inform (loc, "valid models are %s", buf);
+	return true;
+      }
+
+  /* If either memory model argument value is invalid use the numerical
+     value of both in the message.  */
+  return warning_at (loc, OPT_Winvalid_memory_model,
+		     "failure memory model %wi cannot be stronger "
+		     "than success memory model %wi for %qD",
+		     fail, sucs, fndecl);
+}
+
+/* Wrapper for the above.  */
+
+void
+pass_waccess::check_atomic_memmodel (gimple *stmt, tree ord_sucs,
+				     tree ord_fail, const unsigned char *valid)
+{
+  if (warning_suppressed_p (stmt, OPT_Winvalid_memory_model))
+    return;
+
+  if (maybe_warn_memmodel (stmt, ord_sucs, ord_fail, valid))
+    return;
+
+  suppress_warning (stmt, OPT_Winvalid_memory_model);
+}
 
 /* Check a call STMT to an atomic or sync built-in.  */
 
@@ -2699,12 +2935,14 @@  pass_waccess::check_atomic_builtin (gcall *stmt)
   /* The size in bytes of the access by the function, and the number
      of the second argument to check (if any).  */
   unsigned bytes = 0, arg2 = UINT_MAX;
+  unsigned sucs_arg = UINT_MAX, fail_arg = UINT_MAX;
+  /* Points to the array of indices of valid memory models.  */
+  const unsigned char *pvalid_models = NULL;
 
   switch (DECL_FUNCTION_CODE (callee))
     {
 #define BUILTIN_ACCESS_SIZE_FNSPEC(N)			\
-      BUILT_IN_ATOMIC_LOAD_ ## N:			\
-    case BUILT_IN_SYNC_FETCH_AND_ADD_ ## N:		\
+      BUILT_IN_SYNC_FETCH_AND_ADD_ ## N:		\
     case BUILT_IN_SYNC_FETCH_AND_SUB_ ## N:		\
     case BUILT_IN_SYNC_FETCH_AND_OR_ ## N:		\
     case BUILT_IN_SYNC_FETCH_AND_AND_ ## N:		\
@@ -2720,8 +2958,16 @@  pass_waccess::check_atomic_builtin (gcall *stmt)
     case BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_ ## N:	\
     case BUILT_IN_SYNC_VAL_COMPARE_AND_SWAP_ ## N:	\
     case BUILT_IN_SYNC_LOCK_RELEASE_ ## N:		\
-    case BUILT_IN_ATOMIC_EXCHANGE_ ## N:		\
+      bytes = N;					\
+      break;						\
+    case BUILT_IN_ATOMIC_LOAD_ ## N:			\
+      pvalid_models = load_models;			\
+      sucs_arg = 1;					\
+      /* FALLTHROUGH */					\
     case BUILT_IN_ATOMIC_STORE_ ## N:			\
+      if (!pvalid_models)				\
+	pvalid_models = store_models;			\
+      /* FALLTHROUGH */					\
     case BUILT_IN_ATOMIC_ADD_FETCH_ ## N:		\
     case BUILT_IN_ATOMIC_SUB_FETCH_ ## N:		\
     case BUILT_IN_ATOMIC_AND_FETCH_ ## N:		\
@@ -2735,9 +2981,21 @@  pass_waccess::check_atomic_builtin (gcall *stmt)
     case BUILT_IN_ATOMIC_FETCH_OR_ ## N:		\
     case BUILT_IN_ATOMIC_FETCH_XOR_ ## N:		\
 	bytes = N;					\
+	if (sucs_arg == UINT_MAX)			\
+	  sucs_arg = 2;					\
+	if (!pvalid_models)				\
+	  pvalid_models = all_models;			\
+	break;						\
+    case BUILT_IN_ATOMIC_EXCHANGE_ ## N:		\
+	bytes = N;					\
+	sucs_arg = 3;					\
+	pvalid_models = xchg_models;			\
 	break;						\
     case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_ ## N:	\
 	bytes = N;					\
+	sucs_arg = 4;					\
+	fail_arg = 5;					\
+	pvalid_models = all_models;			\
 	arg2 = 1
 
     case BUILTIN_ACCESS_SIZE_FNSPEC (1);
@@ -2751,10 +3009,28 @@  pass_waccess::check_atomic_builtin (gcall *stmt)
     case BUILTIN_ACCESS_SIZE_FNSPEC (16);
       break;
 
+    case BUILT_IN_ATOMIC_CLEAR:
+      sucs_arg = 1;
+      pvalid_models = flag_clr_models;
+      break;
+
     default:
       return false;
     }
 
+  unsigned nargs = gimple_call_num_args (stmt);
+  if (sucs_arg < nargs)
+    {
+      tree ord_sucs = gimple_call_arg (stmt, sucs_arg);
+      tree ord_fail = NULL_TREE;
+      if (fail_arg < nargs)
+	ord_fail = gimple_call_arg (stmt, fail_arg);
+      check_atomic_memmodel (stmt, ord_sucs, ord_fail, pvalid_models);
+    }
+
+  if (!bytes)
+    return true;
+
   tree size = build_int_cstu (sizetype, bytes);
   tree dst = gimple_call_arg (stmt, 0);
   check_memop_access (stmt, dst, NULL_TREE, size);
diff --git a/gcc/input.c b/gcc/input.c
index 4650547c7c9..bff46894549 100644
--- a/gcc/input.c
+++ b/gcc/input.c
@@ -986,10 +986,11 @@  linemap_client_expand_location_to_spelling_point (location_t loc,
 }
 
 
-/* If LOCATION is in a system header and if it is a virtual location for
-   a token coming from the expansion of a macro, unwind it to the
-   location of the expansion point of the macro.  Otherwise, just return
-   LOCATION.
+/* If LOCATION is in a system header and if it is a virtual location
+   for a token coming from the expansion of a macro, unwind it to
+   the location of the expansion point of the macro.  If the expansion
+   point is also in a system header return the original LOCATION.
+   Otherwise, return the location of the expansion point.
 
    This is used for instance when we want to emit diagnostics about a
    token that may be located in a macro that is itself defined in a
@@ -1001,11 +1002,13 @@  linemap_client_expand_location_to_spelling_point (location_t loc,
 location_t
 expansion_point_location_if_in_system_header (location_t location)
 {
-  if (in_system_header_at (location))
-    location = linemap_resolve_location (line_table, location,
-					 LRK_MACRO_EXPANSION_POINT,
-					 NULL);
-  return location;
+  if (!in_system_header_at (location))
+    return location;
+
+  location_t xloc = linemap_resolve_location (line_table, location,
+					      LRK_MACRO_EXPANSION_POINT,
+					      NULL);
+  return in_system_header_at (xloc) ? location : xloc;
 }
 
 /* If LOCATION is a virtual location for a token coming from the expansion
diff --git a/gcc/testsuite/c-c++-common/Winvalid-memory-model.c b/gcc/testsuite/c-c++-common/Winvalid-memory-model.c
new file mode 100644
index 00000000000..474ea5621dc
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Winvalid-memory-model.c
@@ -0,0 +1,239 @@ 
+/* PR middle-end/99612 - Missing warning on incorrect memory order without
+   -Wsystem-headers
+   Verify that constants are propagated through calls to inline functions
+   even at -O0.
+   Also verify that the informational notes after each warning mention
+   the valid memore models for each function.
+   { dg-do compile }
+   { dg-options "-O0 -ftrack-macro-expansion=0" } */
+
+#if !__cplusplus
+# define bool _Bool
+#endif
+
+extern int ei;
+
+static __attribute__ ((always_inline)) inline
+int retval (int val)
+{
+  return val;
+}
+
+void test_load (int *pi)
+{
+  int relaxed = retval (__ATOMIC_RELAXED);
+  *pi++ = __atomic_load_n (&ei, relaxed);
+
+  int consume = retval (__ATOMIC_CONSUME);
+  *pi++ = __atomic_load_n (&ei, consume);
+
+  int acquire = retval (__ATOMIC_ACQUIRE);
+  *pi++ = __atomic_load_n (&ei, acquire);
+
+  int release = retval (__ATOMIC_RELEASE);
+  *pi++ = __atomic_load_n (&ei, release);   // { dg-warning "invalid memory model 'memory_order_release'" }
+  // { dg-message "valid models are 'memory_order_relaxed', 'memory_order_seq_cst', 'memory_order_acquire', 'memory_order_consume'" "note" { target *-*-* } .-1 }
+
+  int acq_rel = retval (__ATOMIC_ACQ_REL);
+  *pi++ = __atomic_load_n (&ei, acq_rel);   // { dg-warning "invalid memory model 'memory_order_acq_rel'" }
+
+  int seq_cst = retval (__ATOMIC_SEQ_CST);
+  *pi++ = __atomic_load_n (&ei, seq_cst);
+
+  /* Verify a nonconstant range.  */
+  int r0_1 = *pi++;
+  if (r0_1 < 0 || 1 < r0_1)
+    r0_1 = 0;
+  *pi++ = __atomic_load_n (&ei, r0_1);
+
+  /* Verify an unbounded range.  */
+  int unknown = *pi++;
+  *pi++ = __atomic_load_n (&ei, unknown);
+}
+
+
+void test_store (int *pi, int x)
+{
+  int relaxed = retval (__ATOMIC_RELAXED);
+  __atomic_store_n (pi++, x, relaxed);
+
+  int consume = retval (__ATOMIC_CONSUME);
+  __atomic_store_n (pi++, x, consume);      // { dg-warning "invalid memory model 'memory_order_consume'" }
+  // { dg-message "valid models are 'memory_order_relaxed', 'memory_order_seq_cst', 'memory_order_release'" "note" { target *-*-* } .-1 }
+
+  int acquire = retval (__ATOMIC_ACQUIRE);
+  __atomic_store_n (pi++, x, acquire);      // { dg-warning "invalid memory model 'memory_order_acquire'" }
+
+  int release = retval (__ATOMIC_RELEASE);
+  __atomic_store_n (pi++, x, release);
+
+  int acq_rel = retval (__ATOMIC_ACQ_REL);
+  __atomic_store_n (pi++, x, acq_rel);      // { dg-warning "invalid memory model 'memory_order_acq_rel'" }
+
+  int seq_cst = retval (__ATOMIC_SEQ_CST);
+  __atomic_store_n (pi++, x, seq_cst);
+
+  int unknown = *pi++;
+  __atomic_store_n (pi++, x, unknown);
+}
+
+
+/* All memory models are valid.  */
+
+void test_exchange (int *pi, int x)
+{
+  int relaxed = retval (__ATOMIC_RELAXED);
+  __atomic_exchange_n (pi++, x, relaxed);
+
+  int consume = retval (__ATOMIC_CONSUME);
+  __atomic_exchange_n (pi++, x, consume);
+
+  int acquire = retval (__ATOMIC_ACQUIRE);
+  __atomic_exchange_n (pi++, x, acquire);
+
+  int release = retval (__ATOMIC_RELEASE);
+  __atomic_exchange_n (pi++, x, release);
+
+  int acq_rel = retval (__ATOMIC_ACQ_REL);
+  __atomic_exchange_n (pi++, x, acq_rel);
+
+  int seq_cst = retval (__ATOMIC_SEQ_CST);
+  __atomic_exchange_n (pi++, x, seq_cst);
+
+  int unknown = *pi++;
+  __atomic_exchange_n (pi++, x, unknown);
+}
+
+
+void test_compare_exchange (int *pi, int *pj, bool weak)
+{
+#define cmpxchg(x, expect, desire, sucs_ord, fail_ord) \
+  __atomic_compare_exchange_n (x, expect, desire, weak, sucs_ord, fail_ord)
+
+  int relaxed = retval (__ATOMIC_RELAXED);
+  cmpxchg (&ei, pi++, *pj++, relaxed, relaxed);
+
+  int consume = retval (__ATOMIC_CONSUME);
+  cmpxchg (&ei, pi++, *pj++, relaxed, consume);   // { dg-warning "failure memory model 'memory_order_consume' cannot be stronger than success memory model 'memory_order_relaxed'" }
+
+  int acquire = retval (__ATOMIC_ACQUIRE);
+  cmpxchg (&ei, pi++, *pj++, relaxed, acquire);   // { dg-warning "failure memory model 'memory_order_acquire' cannot be stronger than success memory model 'memory_order_relaxed'" }
+
+  int release = retval (__ATOMIC_RELEASE);
+  cmpxchg (&ei, pi++, *pj++, relaxed, release);   // { dg-warning "invalid failure memory model 'memory_order_release'" }
+
+  int acq_rel = retval (__ATOMIC_ACQ_REL);
+  cmpxchg (&ei, pi++, *pj++, relaxed, acq_rel);   // { dg-warning "invalid failure memory model 'memory_order_acq_rel'" }
+
+  int seq_cst = retval (__ATOMIC_SEQ_CST);
+  cmpxchg (&ei, pi++, *pj++, relaxed, seq_cst);   // { dg-warning "failure memory model 'memory_order_seq_cst' cannot be stronger than success memory model 'memory_order_relaxed'" }
+
+
+  cmpxchg (&ei, pi++, *pj++, consume, relaxed);
+  cmpxchg (&ei, pi++, *pj++, consume, consume);
+  cmpxchg (&ei, pi++, *pj++, consume, acquire);   // { dg-warning "failure memory model 'memory_order_acquire' cannot be stronger than success memory model 'memory_order_consume'" }
+  cmpxchg (&ei, pi++, *pj++, consume, release);   // { dg-warning "invalid failure memory model 'memory_order_release'" }
+  cmpxchg (&ei, pi++, *pj++, consume, acq_rel);   // { dg-warning "invalid failure memory model 'memory_order_acq_rel'" }
+  cmpxchg (&ei, pi++, *pj++, consume, seq_cst);   // { dg-warning "failure memory model 'memory_order_seq_cst' cannot be stronger than success memory model 'memory_order_consume'" }
+
+  cmpxchg (&ei, pi++, *pj++, acquire, relaxed);
+  cmpxchg (&ei, pi++, *pj++, acquire, consume);
+  cmpxchg (&ei, pi++, *pj++, acquire, acquire);
+  cmpxchg (&ei, pi++, *pj++, acquire, release);   // { dg-warning "invalid failure memory model 'memory_order_release'" }
+  cmpxchg (&ei, pi++, *pj++, acquire, acq_rel);   // { dg-warning "invalid failure memory model 'memory_order_acq_rel'" }
+  cmpxchg (&ei, pi++, *pj++, acquire, seq_cst);   // { dg-warning "failure memory model 'memory_order_seq_cst' cannot be stronger than success memory model 'memory_order_acquire'" }
+
+  cmpxchg (&ei, pi++, *pj++, release, relaxed);
+  cmpxchg (&ei, pi++, *pj++, release, consume);
+  cmpxchg (&ei, pi++, *pj++, release, acquire);
+  cmpxchg (&ei, pi++, *pj++, release, release);   // { dg-warning "invalid failure memory model 'memory_order_release'" }
+  cmpxchg (&ei, pi++, *pj++, release, acq_rel);   // { dg-warning "invalid failure memory model 'memory_order_acq_rel'" }
+  cmpxchg (&ei, pi++, *pj++, release, seq_cst);   // { dg-warning "failure memory model 'memory_order_seq_cst' cannot be stronger than success memory model 'memory_order_release'" }
+
+  cmpxchg (&ei, pi++, *pj++, acq_rel, relaxed);
+  cmpxchg (&ei, pi++, *pj++, acq_rel, consume);
+  cmpxchg (&ei, pi++, *pj++, acq_rel, acquire);
+  cmpxchg (&ei, pi++, *pj++, acq_rel, release);   // { dg-warning "invalid failure memory model 'memory_order_release'" }
+  cmpxchg (&ei, pi++, *pj++, acq_rel, acq_rel);   // { dg-warning "invalid failure memory model 'memory_order_acq_rel'" }
+  cmpxchg (&ei, pi++, *pj++, acq_rel, seq_cst);   // { dg-warning "failure memory model 'memory_order_seq_cst' cannot be stronger than success memory model 'memory_order_acq_rel'" }
+
+  cmpxchg (&ei, pi++, *pj++, seq_cst, relaxed);
+  cmpxchg (&ei, pi++, *pj++, seq_cst, consume);
+  cmpxchg (&ei, pi++, *pj++, seq_cst, acquire);
+  cmpxchg (&ei, pi++, *pj++, seq_cst, release);   // { dg-warning "invalid failure memory model 'memory_order_release'" }
+  cmpxchg (&ei, pi++, *pj++, seq_cst, acq_rel);   // { dg-warning "invalid failure memory model 'memory_order_acq_rel'" }
+  cmpxchg (&ei, pi++, *pj++, seq_cst, seq_cst);
+
+  int unknown = *pi++;
+  cmpxchg (&ei, pi++, *pj++, unknown, seq_cst);
+  cmpxchg (&ei, pi++, *pj++, relaxed, unknown);
+}
+
+
+/* All memory models are valid.  */
+
+void test_add_fetch (unsigned *pi, unsigned x)
+{
+  int relaxed = retval (__ATOMIC_RELAXED);
+  __atomic_add_fetch (pi++, x, relaxed);
+
+  int consume = retval (__ATOMIC_CONSUME);
+  __atomic_add_fetch (pi++, x, consume);
+
+  int acquire = retval (__ATOMIC_ACQUIRE);
+  __atomic_add_fetch (pi++, x, acquire);
+
+  int release = retval (__ATOMIC_RELEASE);
+  __atomic_add_fetch (pi++, x, release);
+
+  int acq_rel = retval (__ATOMIC_ACQ_REL);
+  __atomic_add_fetch (pi++, x, acq_rel);
+
+  int seq_cst = retval (__ATOMIC_SEQ_CST);
+  __atomic_add_fetch (pi++, x, seq_cst);
+
+  int invalid;
+  if (x & 1)
+    {
+      invalid = retval (123);
+      __atomic_add_fetch (pi++, x, invalid);  // { dg-warning "invalid memory model 123 for '\(unsigned int \)?__atomic_add_fetch" }
+    }
+  else
+    {
+      invalid = retval (456);
+      __atomic_add_fetch (pi++, x, invalid);  // { dg-warning "invalid memory model 456 for '\(unsigned int \)?__atomic_add_fetch" }
+    }
+}
+
+void test_sub_fetch (unsigned *pi, unsigned x)
+{
+  int relaxed = retval (__ATOMIC_RELAXED);
+  __atomic_sub_fetch (pi++, x, relaxed);
+
+  int consume = retval (__ATOMIC_CONSUME);
+  __atomic_sub_fetch (pi++, x, consume);
+
+  int acquire = retval (__ATOMIC_ACQUIRE);
+  __atomic_sub_fetch (pi++, x, acquire);
+
+  int release = retval (__ATOMIC_RELEASE);
+  __atomic_sub_fetch (pi++, x, release);
+
+  int acq_rel = retval (__ATOMIC_ACQ_REL);
+  __atomic_sub_fetch (pi++, x, acq_rel);
+
+  int seq_cst = retval (__ATOMIC_SEQ_CST);
+  __atomic_sub_fetch (pi++, x, seq_cst);
+
+  int invalid;
+  if (x & 1)
+    {
+      invalid = retval (123);
+      __atomic_sub_fetch (pi++, x, invalid);  // { dg-warning "invalid memory model 123 for '\(unsigned int \)?__atomic_sub_fetch" }
+    }
+  else
+    {
+      invalid = retval (456);
+      __atomic_sub_fetch (pi++, x, invalid);  // { dg-warning "invalid memory model 456 for '\(unsigned int \)?__atomic_sub_fetch" }
+    }
+}
diff --git a/gcc/testsuite/c-c++-common/pr83059.c b/gcc/testsuite/c-c++-common/pr83059.c
index 44ff67c879d..795faa519ec 100644
--- a/gcc/testsuite/c-c++-common/pr83059.c
+++ b/gcc/testsuite/c-c++-common/pr83059.c
@@ -1,10 +1,13 @@ 
-/* PR c++/83059 */
+/* PR c++/83059 - ICE on invalid C++ code: in tree_to_uhwi, at tree.c:6633 */
 /* { dg-do compile } */
 
 void
 foo (int *p, int *q, int *r)
 {
   __atomic_compare_exchange (p, q, r, 0, 0, -1);	/* { dg-warning "invalid memory model argument 6" } */
-  /* { dg-warning "unknown architecture specifi" "" { target *-*-* } .-1 } */
-  /* { dg-warning "failure memory model cannot be stronger than success memory model" "" { target *-*-* } .-2 } */
 }
+
+/* The test triggers several distinct instance of the warning.  Prune
+   them out; they're not relevant to its main purpose -- to verify
+   there's no ICE.
+   { dg-prune-output "-Winvalid-memory-model" } */
diff --git a/gcc/testsuite/g++.dg/warn/Winvalid-memory-model-2.C b/gcc/testsuite/g++.dg/warn/Winvalid-memory-model-2.C
new file mode 100644
index 00000000000..a15706159aa
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Winvalid-memory-model-2.C
@@ -0,0 +1,79 @@ 
+/* PR middle-end/99612 - Missing warning on incorrect memory order without
+   -Wsystem-headers
+   Verify warnings for atomic functions with optimization.
+   { dg-do compile { target c++11 } }
+   { dg-options "-O1" } */
+
+#include <atomic>
+
+static const std::memory_order relaxed = std::memory_order_relaxed;
+static const std::memory_order consume = std::memory_order_consume;
+static const std::memory_order acquire = std::memory_order_acquire;
+static const std::memory_order release = std::memory_order_release;
+static const std::memory_order acq_rel = std::memory_order_acq_rel;
+static const std::memory_order seq_cst = std::memory_order_seq_cst;
+
+extern std::atomic<int> eai;
+
+void test_load (int *pi)
+{
+  *pi++ = eai.load (relaxed);
+  *pi++ = eai.load (consume);
+  *pi++ = eai.load (acquire);
+  *pi++ = eai.load (release);       // warning
+  *pi++ = eai.load (acq_rel);       // warning
+  *pi++ = eai.load (seq_cst);
+}
+
+/* { dg-regexp " *inlined from \[^\n\r\]+.C:23:.*" "" { target *-*-* } 0 }
+   { dg-regexp " *inlined from \[^\n\r\]+.C:24:.*" "" { target *-*-* } 0 }
+   { dg-warning "__atomic_load\[^\n\r\]* \\\[-Winvalid-memory-model" "warning" { target *-*-* } 0 } */
+
+
+void test_store (int *pi)
+{
+  eai.store (*pi++, relaxed);
+  eai.store (*pi++, consume);       // warning
+  eai.store (*pi++, acquire);       // warning
+  eai.store (*pi++, release);
+  eai.store (*pi++, acq_rel);       // warning
+  eai.store (*pi++, seq_cst);
+}
+
+/* { dg-regexp " *inlined from \[^\n\r\]+.C:36:.*" "" { target *-*-* } 0 }
+   { dg-regexp " *inlined from \[^\n\r\]+.C:37:.*" "" { target *-*-* } 0 }
+   { dg-regexp " *inlined from \[^\n\r\]+.C:39:.*" "" { target *-*-* } 0 }
+   { dg-warning "__atomic_store\[^\n\r]* \\\[-Winvalid-memory-model" "warning" { target *-*-* } 0 } */
+
+
+void test_exchange (const int *pi)
+{
+  eai.exchange (*pi++, relaxed);
+  eai.exchange (*pi++, consume);
+  eai.exchange (*pi++, acquire);
+  eai.exchange (*pi++, release);
+  eai.exchange (*pi++, acq_rel);
+  eai.exchange (*pi++, seq_cst);
+}
+
+
+void test_compare_exchange (int *pi, int *pj)
+{
+#define cmpxchg(x, y, z, o1, o2) \
+  std::atomic_compare_exchange_weak_explicit (x, y, z, o1, o2)
+
+  cmpxchg (&eai, pi++, *pj++, relaxed, relaxed);
+  cmpxchg (&eai, pi++, *pj++, relaxed, consume);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, acquire);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, release);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, acq_rel);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, seq_cst);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, relaxed);
+
+  /* { dg-regexp " *inlined from \[^\n\r\]+.C:66:.*" "" { target *-*-* } 0 }
+     { dg-regexp " *inlined from \[^\n\r\]+.C:67:.*" "" { target *-*-* } 0 }
+     { dg-regexp " *inlined from \[^\n\r\]+.C:68:.*" "" { target *-*-* } 0 }
+     { dg-regexp " *inlined from \[^\n\r\]+.C:69:.*" "" { target *-*-* } 0 }
+     { dg-regexp " *inlined from \[^\n\r\]+.C:70:.*" "" { target *-*-* } 0 }
+     { dg-warning "__atomic_compare_exchange\[^\n\r\]* \\\[-Winvalid-memory-model" "cmpxchg 1" { target *-*-* } 0 } */
+}
diff --git a/gcc/testsuite/g++.dg/warn/Winvalid-memory-model.C b/gcc/testsuite/g++.dg/warn/Winvalid-memory-model.C
new file mode 100644
index 00000000000..5357d540e50
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Winvalid-memory-model.C
@@ -0,0 +1,84 @@ 
+/* PR middle-end/99612 - Missing warning on incorrect memory order without
+   -Wsystem-headers
+   Verify warings for basic atomic functions with no optimization.
+   { dg-do compile { target c++11 } }
+   { dg-options "-O0 -Wall" } */
+
+#include <atomic>
+
+static const std::memory_order relaxed = std::memory_order_relaxed;
+static const std::memory_order consume = std::memory_order_consume;
+static const std::memory_order acquire = std::memory_order_acquire;
+static const std::memory_order release = std::memory_order_release;
+static const std::memory_order acq_rel = std::memory_order_acq_rel;
+static const std::memory_order seq_cst = std::memory_order_seq_cst;
+
+extern std::atomic<int> eai;
+
+void test_load (int *pi)
+{
+  *pi++ = eai.load (relaxed);
+  *pi++ = eai.load (consume);
+  *pi++ = eai.load (acquire);
+  *pi++ = eai.load (release);       // warning
+  *pi++ = eai.load (acq_rel);       // warning
+  *pi++ = eai.load (seq_cst);
+}
+
+/* { dg-regexp " *inlined from \[^\n\r\]+.C:23:.*" "" { target *-*-* } 0 }
+   { dg-regexp " *inlined from \[^\n\r\]+.C:24:.*" "" { target *-*-* } 0 }
+   { dg-warning "__atomic_load\[^\n\r\]* \\\[-Winvalid-memory-model" "warning" { target *-*-* } 0 } */
+
+
+void test_store (int *pi)
+{
+  eai.store (*pi++, relaxed);
+  eai.store (*pi++, consume);       // warning
+  eai.store (*pi++, acquire);       // warning
+  eai.store (*pi++, release);
+  eai.store (*pi++, acq_rel);       // warning
+  eai.store (*pi++, seq_cst);
+}
+
+/* { dg-regexp " *inlined from \[^\n\r\]+.C:36:.*" "" { target *-*-* } 0 }
+   { dg-regexp " *inlined from \[^\n\r\]+.C:37:.*" "" { target *-*-* } 0 }
+   { dg-regexp " *inlined from \[^\n\r\]+.C:39:.*" "" { target *-*-* } 0 }
+   { dg-warning "__atomic_store\[^\n\r]* \\\[-Winvalid-memory-model" "warning" { target *-*-* } 0 } */
+
+
+void test_exchange (const int *pi)
+{
+  eai.exchange (*pi++, relaxed);
+  eai.exchange (*pi++, consume);
+  eai.exchange (*pi++, acquire);
+  eai.exchange (*pi++, release);
+  eai.exchange (*pi++, acq_rel);
+  eai.exchange (*pi++, seq_cst);
+}
+
+/* The following tests fail because std::atomic_compare_exchange_weak_explicit
+   is not declared with attribute always_inline (like the member functions
+   above are).  */
+
+void test_compare_exchange (int *pi, int *pj)
+{
+#define cmpxchg(x, y, z, o1, o2) \
+  std::atomic_compare_exchange_weak_explicit (x, y, z, o1, o2)
+
+  cmpxchg (&eai, pi++, *pj++, relaxed, relaxed);
+  cmpxchg (&eai, pi++, *pj++, relaxed, consume);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, acquire);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, release);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, acq_rel);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, seq_cst);  // warning
+  cmpxchg (&eai, pi++, *pj++, relaxed, relaxed);
+
+  /* HACK: xfail doesn't seem to work for the dg-regexp directives below,
+     so disable them by prepending an X to their names...
+    { Xdg-regexp " *inlined from \[^\n\r\]+.C:66:.*" "" { xfail *-*-* } 0 }
+    { Xdg-regexp " *inlined from \[^\n\r\]+.C:67:.*" "" { xfail *-*-* } 0 }
+    { Xdg-regexp " *inlined from \[^\n\r\]+.C:68:.*" "" { xfail *-*-* } 0 }
+    { Xdg-regexp " *inlined from \[^\n\r\]+.C:69:.*" "" { xfail *-*-* } 0 }
+    { Xdg-regexp " *inlined from \[^\n\r\]+.C:70:.*" "" { xfail *-*-* } 0 }
+    { dg-warning "__atomic_compare_exchange\[^\n\r\]* \\\[-Winvalid-memory-model" "cmpxchg 1" { xfail *-*-* } 0 } */
+}
diff --git a/gcc/testsuite/gcc.dg/atomic-invalid-2.c b/gcc/testsuite/gcc.dg/atomic-invalid-2.c
index c73458e9957..220432c34d3 100644
--- a/gcc/testsuite/gcc.dg/atomic-invalid-2.c
+++ b/gcc/testsuite/gcc.dg/atomic-invalid-2.c
@@ -38,13 +38,13 @@  exchange (atomic_int *i)
 {
   int r;
 
-  atomic_compare_exchange_strong_explicit (i, &r, 0, memory_order_seq_cst, memory_order_release); /* { dg-warning "invalid failure memory" } */
-  atomic_compare_exchange_strong_explicit (i, &r, 0, memory_order_seq_cst, memory_order_acq_rel); /* { dg-warning "invalid failure memory" } */
-  atomic_compare_exchange_strong_explicit (i, &r, 0, memory_order_relaxed, memory_order_consume); /* { dg-warning "failure memory model cannot be stronger" } */
+  atomic_compare_exchange_strong_explicit (i, &r, 0, memory_order_seq_cst, memory_order_release); /* { dg-warning "invalid failure memory model 'memory_order_release'" } */
+  atomic_compare_exchange_strong_explicit (i, &r, 0, memory_order_seq_cst, memory_order_acq_rel); /* { dg-warning "invalid failure memory model 'memory_order_acq_rel'" } */
+  atomic_compare_exchange_strong_explicit (i, &r, 0, memory_order_relaxed, memory_order_consume); /* { dg-warning "failure memory model 'memory_order_consume' cannot be stronger than success memory model 'memory_order_relaxed'" } */
 
-  atomic_compare_exchange_weak_explicit (i, &r, 0, memory_order_seq_cst, memory_order_release); /* { dg-warning "invalid failure memory" } */
-  atomic_compare_exchange_weak_explicit (i, &r, 0, memory_order_seq_cst, memory_order_acq_rel); /* { dg-warning "invalid failure memory" } */
-  atomic_compare_exchange_weak_explicit (i, &r, 0, memory_order_relaxed, memory_order_consume); /* { dg-warning "failure memory model cannot be stronger" } */
+  atomic_compare_exchange_weak_explicit (i, &r, 0, memory_order_seq_cst, memory_order_release); /* { dg-warning "invalid failure memory model 'memory_order_release'" } */
+  atomic_compare_exchange_weak_explicit (i, &r, 0, memory_order_seq_cst, memory_order_acq_rel); /* { dg-warning "invalid failure memory model 'memory_order_acq_rel'" } */
+  atomic_compare_exchange_weak_explicit (i, &r, 0, memory_order_relaxed, memory_order_consume); /* { dg-warning "failure memory model 'memory_order_consume' cannot be stronger than success memory model 'memory_order_relaxed'" } */
 }
 
 /* atomic_flag_clear():
diff --git a/gcc/testsuite/gcc.dg/atomic-invalid.c b/gcc/testsuite/gcc.dg/atomic-invalid.c
index f2adcdfbfa8..26da5b725e3 100644
--- a/gcc/testsuite/gcc.dg/atomic-invalid.c
+++ b/gcc/testsuite/gcc.dg/atomic-invalid.c
@@ -13,7 +13,7 @@  bool x;
 int
 main ()
 {
-  __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST); /* { dg-warning "failure memory model cannot be stronger" } */
+  __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST); /* { dg-warning "failure memory model 'memory_order_seq_cst' cannot be stronger" } */
   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_SEQ_CST, __ATOMIC_RELEASE); /* { dg-warning "invalid failure memory" } */
   __atomic_compare_exchange_n (&i, &e, 1, 1, __ATOMIC_SEQ_CST, __ATOMIC_ACQ_REL); /* { dg-warning "invalid failure memory" } */
 
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index ef654d7b878..df16ced892d 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -12409,9 +12409,7 @@  This built-in function implements an atomic exchange operation.  It writes
 @var{val} into @code{*@var{ptr}}, and returns the previous contents of
 @code{*@var{ptr}}.
 
-The valid memory order variants are
-@code{__ATOMIC_RELAXED}, @code{__ATOMIC_SEQ_CST}, @code{__ATOMIC_ACQUIRE},
-@code{__ATOMIC_RELEASE}, and @code{__ATOMIC_ACQ_REL}.
+All memory order variants are valid.
 
 @end deftypefn