[Sema][ObjC] Loosen restrictions on reinterpret_cast involving indirect ARC-managed pointers

[ahatanak]

Allow using reinterpret_cast for conversions between indirect ARC pointers and other pointer types.

rdar://152905399

[llvmbot]

@llvm/pr-subscribers-clang

Author: Akira Hatanaka (ahatanak)

Changes

Allow using reinterpret_cast for conversions between indirect ARC pointers and other pointer types.

rdar://153049268

---

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

4 Files Affected:

• (modified) clang/include/clang/Sema/SemaObjC.h (+2-1)
• (modified) clang/lib/Sema/SemaCast.cpp (+7-4)
• (modified) clang/lib/Sema/SemaExprObjC.cpp (+10-4)
• (modified) clang/test/SemaObjCXX/arc-type-conversion.mm (+12-2)

diff --git a/clang/include/clang/Sema/SemaObjC.h b/clang/include/clang/Sema/SemaObjC.h
index b629c6d291402..ed08ff0acf89d 100644
--- a/clang/include/clang/Sema/SemaObjC.h
+++ b/clang/include/clang/Sema/SemaObjC.h
@@ -812,7 +812,8 @@ class SemaObjC : public SemaBase {
                                           CheckedConversionKind CCK,
                                           bool Diagnose = true,
                                           bool DiagnoseCFAudited = false,
-                                          BinaryOperatorKind Opc = BO_PtrMemD);
+                                          BinaryOperatorKind Opc = BO_PtrMemD,
+                                          bool IsReinterpretCast = false);
 
   Expr *stripARCUnbridgedCast(Expr *e);
   void diagnoseARCUnbridgedCast(Expr *e);
diff --git a/clang/lib/Sema/SemaCast.cpp b/clang/lib/Sema/SemaCast.cpp
index 14e16bc39eb3a..e15a43c116516 100644
--- a/clang/lib/Sema/SemaCast.cpp
+++ b/clang/lib/Sema/SemaCast.cpp
@@ -161,12 +161,14 @@ namespace {
       Self.CheckCastAlign(SrcExpr.get(), DestType, OpRange);
     }
 
-    void checkObjCConversion(CheckedConversionKind CCK) {
+    void checkObjCConversion(CheckedConversionKind CCK,
+                             bool IsReinterpretCast = false) {
       assert(Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers());
 
       Expr *src = SrcExpr.get();
-      if (Self.ObjC().CheckObjCConversion(OpRange, DestType, src, CCK) ==
-          SemaObjC::ACR_unbridged)
+      if (Self.ObjC().CheckObjCConversion(
+              OpRange, DestType, src, CCK, true, false, BO_PtrMemD,
+              IsReinterpretCast) == SemaObjC::ACR_unbridged)
         IsARCUnbridgedCast = true;
       SrcExpr = src;
     }
@@ -1263,7 +1265,8 @@ void CastOperation::CheckReinterpretCast() {
 
   if (isValidCast(tcr)) {
     if (Self.getLangOpts().allowsNonTrivialObjCLifetimeQualifiers())
-      checkObjCConversion(CheckedConversionKind::OtherCast);
+      checkObjCConversion(CheckedConversionKind::OtherCast,
+                          /*IsReinterpretCast=*/true);
     DiagnoseReinterpretUpDownCast(Self, SrcExpr.get(), DestType, OpRange);
 
     if (unsigned DiagID = checkCastFunctionType(Self, SrcExpr, DestType))
diff --git a/clang/lib/Sema/SemaExprObjC.cpp b/clang/lib/Sema/SemaExprObjC.cpp
index 3505d9f38d23c..59a2e660364de 100644
--- a/clang/lib/Sema/SemaExprObjC.cpp
+++ b/clang/lib/Sema/SemaExprObjC.cpp
@@ -4390,7 +4390,7 @@ SemaObjC::ARCConversionResult
 SemaObjC::CheckObjCConversion(SourceRange castRange, QualType castType,
                               Expr *&castExpr, CheckedConversionKind CCK,
                               bool Diagnose, bool DiagnoseCFAudited,
-                              BinaryOperatorKind Opc) {
+                              BinaryOperatorKind Opc, bool IsReinterpretCast) {
   ASTContext &Context = getASTContext();
   QualType castExprType = castExpr->getType();
 
@@ -4450,13 +4450,19 @@ SemaObjC::CheckObjCConversion(SourceRange castRange, QualType castType,
   // must be explicit.
   // Allow conversions between pointers to lifetime types and coreFoundation
   // pointers too, but only when the conversions are explicit.
+  // Allow conversions requested with a reinterpret_cast that converts an
+  // expression of type T* to type U*.
   if (exprACTC == ACTC_indirectRetainable &&
       (castACTC == ACTC_voidPtr ||
-       (castACTC == ACTC_coreFoundation && SemaRef.isCast(CCK))))
+       (castACTC == ACTC_coreFoundation && SemaRef.isCast(CCK)) ||
+       (IsReinterpretCast && (effCastType->isPointerType() ||
+                              effCastType->isObjCObjectPointerType()))))
     return ACR_okay;
   if (castACTC == ACTC_indirectRetainable &&
-      (exprACTC == ACTC_voidPtr || exprACTC == ACTC_coreFoundation) &&
-      SemaRef.isCast(CCK))
+      (((exprACTC == ACTC_voidPtr || exprACTC == ACTC_coreFoundation) &&
+        SemaRef.isCast(CCK)) ||
+       (IsReinterpretCast && (castExprType->isPointerType() ||
+                              castExprType->isObjCObjectPointerType()))))
     return ACR_okay;
 
   switch (ARCCastChecker(Context, exprACTC, castACTC, false).Visit(castExpr)) {
diff --git a/clang/test/SemaObjCXX/arc-type-conversion.mm b/clang/test/SemaObjCXX/arc-type-conversion.mm
index 64cfd02ec18c0..2b44624121504 100644
--- a/clang/test/SemaObjCXX/arc-type-conversion.mm
+++ b/clang/test/SemaObjCXX/arc-type-conversion.mm
@@ -1,5 +1,7 @@
 // RUN: %clang_cc1 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -verify -fblocks %s
 
+@class NSString;
+
 void * cvt(id arg) // expected-note{{candidate function not viable: cannot convert argument of incomplete type 'void *' to '__strong id'}}
 {
   void* voidp_val;
@@ -72,6 +74,16 @@ void test_reinterpret_cast(__strong id *sip, __weak id *wip,
   (void)reinterpret_cast<__weak id *>(cwip); // expected-error{{reinterpret_cast from '__weak id const *' to '__weak id *' casts away qualifiers}}
   (void)reinterpret_cast<__weak id *>(csip); // expected-error{{reinterpret_cast from '__strong id const *' to '__weak id *' casts away qualifiers}}
   (void)reinterpret_cast<__strong id *>(cwip); // expected-error{{reinterpret_cast from '__weak id const *' to '__strong id *' casts away qualifiers}}
+
+  auto *p0 = reinterpret_cast<unsigned long *>(sip);
+  (void)reinterpret_cast<__strong id *>(p0);
+  auto *p1 = reinterpret_cast<__weak NSString *>(sip);
+  (void)reinterpret_cast<__strong id *>(p1);
+  (void)reinterpret_cast<unsigned long *>(csip); // expected-error {{reinterpret_cast from '__strong id const *' to 'unsigned long *' casts away qualifiers}}
+  const unsigned long *p2 = nullptr;
+  (void)reinterpret_cast<__strong id *>(p2); // expected-error {{reinterpret_cast from 'const unsigned long *' to '__strong id *' casts away qualifiers}}
+  auto ul = reinterpret_cast<unsigned long>(sip);
+  (void)reinterpret_cast<__strong id *>(ul); // expected-error {{cast of 'unsigned long' to '__strong id *' is disallowed with ARC}}
 }
 
 void test_cstyle_cast(__strong id *sip, __weak id *wip, 
@@ -194,8 +206,6 @@ void from_void(void *vp) {
 typedef void (^Block_strong)() __strong;
 typedef void (^Block_autoreleasing)() __autoreleasing;
 
-@class NSString;
-
 void ownership_transfer_in_cast(void *vp, Block *pblk) {
   __strong NSString **sip2 = static_cast<NSString **>(static_cast<__strong id *>(vp));
   __strong NSString **&si2pref = static_cast<NSString **&>(sip2);

[rjmccall]

This all looks good to me. Please add a release note saying that we fixed an issue where the implementation was stricter than the ARC specification said. Also, I know that you had some questions around const and other qualifiers while implementing this; did you want to reflect those as test cases?

[rjmccall]

Could you also test that casts which preserve qualifiers work as expected?

[rjmccall]

Thanks, LGTM.