[Clang] Fix crash with implicit int-to-pointer conversion

[ostannard]

If an integer is passed to the pointer argument of the __atomic_test_and_set or __atomic_clear builtins with the int-conversion error disabled or downgraded, we crashed in codegen due to assuming that the type is always a pointer after skip[ping past implicit casts.

Fixes #111293.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Oliver Stannard (ostannard)

Changes

If an integer is passed to the pointer argument of the __atomic_test_and_set or __atomic_clear builtins with the int-conversion error disabled or downgraded, we crashed in codegen due to assuming that the type is always a pointer after skip[ping past implicit casts.

Fixes #111293.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CGBuiltin.cpp (+4-2)
• (modified) clang/test/CodeGen/atomic-ops.c (+7-3)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 65d7f5c54a1913..87955a2c158454 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -4928,8 +4928,9 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
     // Look at the argument type to determine whether this is a volatile
     // operation. The parameter type is always volatile.
     QualType PtrTy = E->getArg(0)->IgnoreImpCasts()->getType();
+    QualType PointeeTy = PtrTy->getPointeeType();
     bool Volatile =
-        PtrTy->castAs<PointerType>()->getPointeeType().isVolatileQualified();
+        PointeeTy.isNull() ? false : PointeeTy.isVolatileQualified();
 
     Address Ptr =
         EmitPointerWithAlignment(E->getArg(0)).withElementType(Int8Ty);
@@ -5011,8 +5012,9 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
 
   case Builtin::BI__atomic_clear: {
     QualType PtrTy = E->getArg(0)->IgnoreImpCasts()->getType();
+    QualType PointeeTy = PtrTy->getPointeeType();
     bool Volatile =
-        PtrTy->castAs<PointerType>()->getPointeeType().isVolatileQualified();
+        PointeeTy.isNull() ? false : PointeeTy.isVolatileQualified();
 
     Address Ptr = EmitPointerWithAlignment(E->getArg(0));
     Ptr = Ptr.withElementType(Int8Ty);
diff --git a/clang/test/CodeGen/atomic-ops.c b/clang/test/CodeGen/atomic-ops.c
index b6060dcc540f90..4c7d674836cd36 100644
--- a/clang/test/CodeGen/atomic-ops.c
+++ b/clang/test/CodeGen/atomic-ops.c
@@ -1,10 +1,10 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map -triple=i686-apple-darwin9 | FileCheck %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -ffreestanding -ffake-address-space-map -triple=i686-apple-darwin9 -Wno-error=int-conversion | FileCheck %s
 // REQUIRES: x86-registered-target
 
 // Also test serialization of atomic operations here, to avoid duplicating the
 // test.
-// RUN: %clang_cc1 %s -emit-pch -o %t -ffreestanding -ffake-address-space-map -triple=i686-apple-darwin9
-// RUN: %clang_cc1 %s -include-pch %t -ffreestanding -ffake-address-space-map -triple=i686-apple-darwin9 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -emit-pch -o %t -ffreestanding -ffake-address-space-map -triple=i686-apple-darwin9 -Wno-error=int-conversion
+// RUN: %clang_cc1 %s -include-pch %t -ffreestanding -ffake-address-space-map -triple=i686-apple-darwin9 -Wno-error=int-conversion -emit-llvm -o - | FileCheck %s
 #ifndef ALREADY_INCLUDED
 #define ALREADY_INCLUDED
 
@@ -310,10 +310,14 @@ void test_and_set(void) {
   __atomic_test_and_set(&flag1, memory_order_seq_cst);
   // CHECK: atomicrmw volatile xchg ptr @flag2, i8 1 acquire, align 1
   __atomic_test_and_set(&flag2, memory_order_acquire);
+  // CHECK: atomicrmw xchg ptr inttoptr (i32 32768 to ptr), i8 1 acquire, align 1
+  __atomic_test_and_set(0x8000, memory_order_acquire);
   // CHECK: store atomic volatile i8 0, ptr @flag2 release, align 1
   __atomic_clear(&flag2, memory_order_release);
   // CHECK: store atomic i8 0, ptr @flag1 seq_cst, align 1
   __atomic_clear(&flag1, memory_order_seq_cst);
+  // CHECK: store atomic i8 0, ptr inttoptr (i32 32768 to ptr) seq_cst, align 1
+  __atomic_clear(0x8000, memory_order_seq_cst);
 }
 
 struct Sixteen {

[efriedma-quic]

Is there some discussion somewhere of why the change from hasErrorOccurred to hasUnrecoverableErrorOccurred happened?

[efriedma-quic]

IgnoreImpCasts is usually wrong; in general, it's going to find an unrelated type.

If __atomic_test_and_set is overloaded to have both a volatile and non-volatile variant, it should be using custom type-checking in Sema.

[efriedma-quic]

Related testcase: void f() { volatile int x[10]; __atomic_test_and_set(x,0); }.

[shafik]

Is there some discussion somewhere of why the change from hasErrorOccurred to hasUnrecoverableErrorOccurred happened?

I had asked about this in the original PR here: #84146 but did not get a response.

[ostannard]

If __atomic_test_and_set is overloaded to have both a volatile and non-volatile variant, it should be using custom type-checking in Sema.

Looking at the code again, there's a separate system used to codegen most of the atomic builtins, so I've switched to using that. That completely replaces this change, so I've started a new PR #120449, and will close this one.

I had asked about this in the original PR here: #84146 but did not get a response.

I've added a comment there.