On Thu, Jan 27, 2022 at 3:47 PM Andrew Pinski <pins...@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
