[clang][Sema] Fix crash on atomic builtins with incomplete type args

[hazohelet]

This patch fixes the crash when pointers to incomplete type are passed to atomic builtins such as __atomic_load.
ASTContext::getTypeInfoInChars assumes that the argument type is a complete type, so I added a check to eliminate cases where incomplete types gets passed to this function

Relevant PR: #91057
Fixes #96289

[llvmbot]

@llvm/pr-subscribers-clang

Author: Takuya Shimizu (hazohelet)

Changes

This patch fixes the crash when pointers to incomplete type are passed to atomic builtins such as __atomic_load.
ASTContext::getTypeInfoInChars assumes that the argument type is a complete type, so I added a check to eliminate cases where incomplete types gets passed to this function

Relevant PR: #91057
Fixes #96289

---

Full diff: https://github.com/llvm/llvm-project/pull/96374.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaChecking.cpp (+2-1)
• (modified) clang/test/Sema/atomic-ops.c (+30)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 7a2076d139c69..1872bdb5767f0 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -4570,7 +4570,8 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
   }
 
   // Pointer to object of size zero is not allowed.
-  if (Context.getTypeInfoInChars(AtomTy).Width.isZero()) {
+  if (!AtomTy->isIncompleteType() &&
+      Context.getTypeInfoInChars(AtomTy).Width.isZero()) {
     Diag(ExprRange.getBegin(), diag::err_atomic_builtin_must_be_pointer)
         << Ptr->getType() << 1 << Ptr->getSourceRange();
     return ExprError();
diff --git a/clang/test/Sema/atomic-ops.c b/clang/test/Sema/atomic-ops.c
index 2024b81ce6aec..d957461b6cb75 100644
--- a/clang/test/Sema/atomic-ops.c
+++ b/clang/test/Sema/atomic-ops.c
@@ -671,6 +671,36 @@ void zeroSizeArgError(struct Z *a, struct Z *b, struct Z *c) {
 
 }
 
+struct IncompleteTy IncA, IncB, IncC; // expected-error 3{{tentative definition has type 'struct IncompleteTy' that is never completed}} \
+                                      // expected-note 3{{forward declaration of 'struct IncompleteTy'}}
+void incompleteTypeArgError() {
+  __atomic_exchange(&IncB, &IncB, &IncC, memory_order_relaxed); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_exchange(&IncB, &IncB, &IncC, memory_order_acq_rel); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_exchange(&IncB, &IncB, &IncC, memory_order_acquire); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_exchange(&IncB, &IncB, &IncC, memory_order_consume); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_exchange(&IncB, &IncB, &IncC, memory_order_release); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_exchange(&IncB, &IncB, &IncC, memory_order_seq_cst); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_load(&IncA, &IncB, memory_order_relaxed); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_load(&IncA, &IncB, memory_order_acq_rel); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_load(&IncA, &IncB, memory_order_acquire); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_load(&IncA, &IncB, memory_order_consume); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_load(&IncA, &IncB, memory_order_release); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_load(&IncA, &IncB, memory_order_seq_cst); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_store(&IncA, &IncB, memory_order_relaxed); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_store(&IncA, &IncB, memory_order_acq_rel); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_store(&IncA, &IncB, memory_order_acquire); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_store(&IncA, &IncB, memory_order_consume); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_store(&IncA, &IncB, memory_order_release); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_store(&IncA, &IncB, memory_order_seq_cst); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_compare_exchange(&IncA, &IncB, &IncC, 0, memory_order_relaxed, memory_order_relaxed); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_compare_exchange(&IncA, &IncB, &IncC, 0, memory_order_acq_rel, memory_order_acq_rel); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_compare_exchange(&IncA, &IncB, &IncC, 0, memory_order_acquire, memory_order_acquire); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_compare_exchange(&IncA, &IncB, &IncC, 0, memory_order_consume, memory_order_consume); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_compare_exchange(&IncA, &IncB, &IncC, 0, memory_order_release, memory_order_release); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_compare_exchange(&IncA, &IncB, &IncC, 0, memory_order_seq_cst, memory_order_seq_cst); // expected-error {{must be a pointer to a trivially-copyable type}}
+
+}
+
 void nullPointerWarning(void) {
   volatile _Atomic(int) vai;
   _Atomic(int) ai;

[MitalAshok]

Could you knock out a related bug at the same time:

template<typename>
struct X {
  char arr[1];
};

extern X<void>* p, *q;
//X<void> inst;

void f() {
  __atomic_exchange(p, p, q, __ATOMIC_RELAXED);
}

With the line commented out, currently this crashes, but in Clang 18 and with your patch, it fails as X<void> hasn't been instantiated yet and isTriviallyCopyableType is false for incomplete types:

test:10:3: error: address argument to atomic operation must be a pointer to a trivially-copyable type ('X<void> *' invalid)
   10 |   __atomic_exchange(p, p, q, __ATOMIC_RELAXED);
      |   ^                 ~

->isIncompleteType() is true for uninstantiated templates too, so just a RequireCompleteType should stop the crash and instantiate templates as needed.

[Sirraide]

RequireCompleteType seems like the way to go here, yeah.

[AaronBallman]

Thank you for the fix! The changes should also come with a release note in clang/docs/ReleaseNotes.rst so users know about the improvement.

[AaronBallman]

If you call RequireCompleteType() before this if instead of checking for an incomplete type, do you get better diagnostic behavior?

[hazohelet]

The current diagnostic message only says we need trivially-copyable type here, and the RequireCompleteType version would say we need complete type here and it sounds more understandable to me.

[hazohelet]

Thanks for the feedback. RequireCompleteType seems like the right direction.
I'm not writing about the crash fix to the release note because this crash was introduced AFTER clang 18 was released.

[AaronBallman]

LGTM!