[C11] Improve the diagnostic when accessing a member of an atomic struct

[aaron.ballman]

Summary

Member access for an atomic structure or union is unconditional undefined behavior (C11 6.5.2.3p5). However, we would issue a confusing error message about the base expression not being a structure or union type.

GCC issues a warning for this case. Clang now warns as well, but the warning is defaulted to an error because the actual access is still unsafe.

This fixes Issue 54563.

[erichkeane]

clang/lib/Sema/SemaExprMember.cpp

1300

This seems to apply to both C and C++. I guess "_Atomic" is C only, so we get to define its behavior for C++?

What does GCC do in C++ mode?

clang/test/Sema/atomic-expr.c

85

This still catches RHS access as well, right?

Also, does it still 'work' with the non-qualifier version of _Atomic?

[aaron.ballman]

clang/lib/Sema/SemaExprMember.cpp

1300

Clang supports _Atomic in C++ as an extension (with the C semantics), GCC does not.

However, after doing some testing, I actually question whether Clang supports _Atomic in C++ more or just "pretends everything will work out fine in C++ mode but nobody ever actually checked that". I'm not going to add additional test coverage for this because I'm going to see just how many issues I spot in C++ first (it may be a better approach to disable the extension in C++).

clang/test/Sema/atomic-expr.c

85

This still catches RHS access as well, right?

Yes, I'll add tests.

Also, does it still 'work' with the non-qualifier version of _Atomic?

Yes, I'll add a test.

[aaron.ballman]

Added test cases based on review feedback.

[erichkeane]

clang/lib/Sema/SemaExprMember.cpp

1300

Based on the examples I've seen, I think we're better off matching GCC's behavior here of disallowing _Atomic in both forms in C++, unless it is necessary/used to implement the std::atomic version in libc++ (obviously it isn't in libstdc++ if they disable it in c++ mode?).

[aaron.ballman]

Thanks for the review, I've commit in 3c84e4a0dbd08fc03bbcdd8354a984e0efcf7672

clang/docs/ReleaseNotes.rst

- No longer crash when specifying a variably-modified parameter type in a
   `Issue 50541 <https://github.com/llvm/llvm-project/issues/50541>`_.
 - Allow multiple ``#pragma weak`` directives to name the same undeclared (if an
   alias, target) identifier instead of only processing one such ``#pragma weak``
   per identifier.
   Fixes `Issue 28985 <https://github.com/llvm/llvm-project/issues/28985>`_.
 - Assignment expressions in C11 and later mode now properly strip the _Atomic
   qualifier when determining the type of the assignment expression. Fixes
   `Issue 48742 <https://github.com/llvm/llvm-project/issues/48742>`_.
+- Improved the diagnostic when accessing a member of an atomic structure or
+  union object in C; was previously an unhelpful error, but now issues a
+  `-Watomic-access` warning which defaults to an error. Fixes
+  `Issue 54563 <https://github.com/llvm/llvm-project/issues/54563>`_.
 - Unevaluated lambdas in dependant contexts no longer result in clang crashing.
   This fixes Issues `50376 <https://github.com/llvm/llvm-project/issues/50376>`_,
   `51414 <https://github.com/llvm/llvm-project/issues/51414>`_,
   `51416 <https://github.com/llvm/llvm-project/issues/51416>`_,
   and `51641 <https://github.com/llvm/llvm-project/issues/51641>`_.
 
 
 Improvements to Clang's diagnostics

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "non-void block should return a value">;
 def err_func_def_incomplete_result : Error<
   "incomplete result type %0 in function definition">;
 def err_atomic_specifier_bad_type
     : Error<"_Atomic cannot be applied to "
             "%select{incomplete |array |function |reference |atomic |qualified "
             "|sizeless ||integer }0type "
             "%1 %select{|||||||which is not trivially copyable|}0">;
+def warn_atomic_member_access : Warning<
+  "accessing a member of an atomic structure or union is undefined behavior">,
+  InGroup<DiagGroup<"atomic-access">>, DefaultError;
 
 // Expressions.
 def ext_sizeof_alignof_function_type : Extension<
   "invalid application of '%0' to a function type">, InGroup<PointerArith>;
 def ext_sizeof_alignof_void_type : Extension<
   "invalid application of '%0' to a void type">, InGroup<PointerArith>;
 def err_opencl_sizeof_alignof_type : Error<
   "invalid application of '%0' to a void type">;

clang/lib/Sema/SemaExprMember.cpp

       goto fail;
     } else {
       S.Diag(MemberLoc, diag::err_typecheck_member_reference_arrow)
         << BaseType << BaseExpr.get()->getSourceRange();
       return ExprError();
     }
   }
 
+  // If the base type is an atomic type, this access is undefined behavior per
+  // C11 6.5.2.3p5. Instead of giving a typecheck error, we'll warn the user
+  // about the UB and recover by converting the atomic lvalue into a non-atomic
+  // lvalue. Because this is inherently unsafe as an atomic operation, the
+  // warning defaults to an error.
+  if (const auto *ATy = BaseType->getAs<AtomicType>()) {
+    S.Diag(OpLoc, diag::warn_atomic_member_access);
+    BaseType = ATy->getValueType().getUnqualifiedType();
+    BaseExpr = ImplicitCastExpr::Create(
+        S.Context, IsArrow ? S.Context.getPointerType(BaseType) : BaseType,
+        CK_AtomicToNonAtomic, BaseExpr.get(), nullptr,
+        BaseExpr.get()->getValueKind(), FPOptionsOverride());
+  }
+
   // Handle field access to simple records.
   if (const RecordType *RTy = BaseType->getAs<RecordType>()) {
     TypoExpr *TE = nullptr;
     if (LookupMemberExprInRecord(S, R, BaseExpr.get(), RTy, OpLoc, IsArrow, SS,
                                  HasTemplateArgs, TemplateKWLoc, TE))
       return ExprError();
 
     // Returning valid-but-null is how we indicate to the caller that

clang/test/Sema/atomic-expr.c

 // RUN: %clang_cc1 %s -verify -fsyntax-only
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -verify=off -fsyntax-only -Wno-atomic-access
+// off-no-diagnostics
 
 _Atomic(unsigned int) data1;
 int _Atomic data2;
 
 // Shift operations
 
 int func_01 (int x) {
   return data1 << x;
void func_15(void) {
   int z = (int)(x = 2);
   y = (x = 2);
   z = (int)(x = 2);
   y = (x += 2);
 
   _Static_assert(__builtin_types_compatible_p(__typeof__(x = 2), int), "incorrect");
   _Static_assert(__builtin_types_compatible_p(__typeof__(x += 2), int), "incorrect");
 }
+
+// Ensure that member access of an atomic structure or union type is properly
+// diagnosed as being undefined behavior; Issue 54563.
+void func_16(void) {
+  // LHS member access.
+  _Atomic struct { int val; } x, *xp;
+  x.val = 12;   // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+  xp->val = 12; // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+
+  _Atomic union {
+    int ival;
+    float fval;
+  } y, *yp;
+  y.ival = 12;     // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+  yp->fval = 1.2f; // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+
+  // RHS member access.
+  int xval = x.val; // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+  xval = xp->val;   // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+  int yval = y.val; // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+  yval = yp->val;   // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+
+  // Using the type specifier instead of the type qualifier.
+  _Atomic(struct { int val; }) z;
+  z.val = 12;       // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+  int zval = z.val; // expected-error {{accessing a member of an atomic structure or union is undefined behavior}}
+}