Fix adress cast for C++ in SEMA

[sfantao]

Summary

The cast of a pointer to a different address space makes clang implement an implicit cast during SEMA that was not getting the right CastKind information. This caused the code emission to fail later on. This patch adds an extra check before the creation of the cast and fixes the kind if the types are pointers and the address spaces differ.

This is a fix for http://llvm.org/bugs/show_bug.cgi?id=20135.

[sfantao]

Ping!

[hfinkel]

lib/Sema/Sema.cpp

398

Add a period to the end of the comment.

399

Does this apply to CK_LValueBitCast too?

[sfantao]

Add period to comment.

[sfantao]

Hi Hal,

Thanks for reviewing the patch! The answer to your remarks is bellow.

Regards,
Samuel

lib/Sema/Sema.cpp

398

Will do!

399

This seems to be specific to the AddressCast. For these kind of casts we were getting for C++:

`-CStyleCastExpr 0x1000b254df8 <col:6, col:26> '__attribute__((address_space(1))) char *' <NoOp>
  `-ImplicitCastExpr 0x1000b254de0 <col:26> '__attribute__((address_space(1))) char *' <NoOp>
    `-ImplicitCastExpr 0x1000b254dc8 <col:26> 'const char *' <ArrayToPointerDecay>
      `-StringLiteral 0x1000b254d58 <col:26> 'const char [2]' lvalue "a"

That kind of implicit cast seems to be generated only for address space casts. The patch just fixes the Noop, so we get:

`-CStyleCastExpr 0x10010154df8 <col:6, col:26> '__attribute__((address_space(1))) char *' <NoOp>
  `-ImplicitCastExpr 0x10010154de0 <col:26> '__attribute__((address_space(1))) char *' <AddressSpaceConversion>
    `-ImplicitCastExpr 0x10010154dc8 <col:26> 'const char *' <ArrayToPointerDecay>
      `-StringLiteral 0x10010154d58 <col:26> 'const char [2]' lvalue "a"

I don't fully understand why a pair of CStyleCastExpr+ImplicitCastExpr is being used instead of a single CStyleCastExpr. I see however, that the pair version seem to fit the Sema implementation better.

[hfinkel]

Also, this needs a test case.

[sfantao]

Hi Hal,

Thanks again for looking into this. Adding a regression test for the issue this patch addresses. Let me know if I created it in the right place.

Thanks,
Samuel

[sfantao]

Ping!

[sfantao]

Any other comments on this patch?

Thanks!
Samuel

[rsmith]

lib/Sema/Sema.cpp

343

Why is Kind == CK_NoOp being passed in here for an address space cast? That seems like it might be a bug in the caller. Do you know which calls to ImpCastExprToType result in this happening?

389–395

If we (somehow) hit this case and have an inner cast of kind CK_NoOp, we'll fail to create a CK_AddressSpaceConversion cast. The added code should be above this block.

test/Sema/address-space-cast.c

1–2 ↗

(On Diff #25808)

Please use the conventional check prefix of CHECK for the first of these, and CHECK-CXX for the second.

Also, it would be preferable to check the generated IR here rather than an AST dump -- the AST dump format is less stable than the textual IR format, and it's much more important that we get the IR right than the dump :-)

[sfantao]

Refactor the patch following rsmith requests.

[sfantao]

lib/Sema/Sema.cpp

343

This is the stack I have when I reach the point where the address cast is performed.

clang::Sema::ImpCastExprToType() at Sema.cpp:370 0x141629b8	
clang::Sema::PerformImplicitConversion() at SemaExprCXX.cpp:3,447 0x1456981c	
clang::Sema::PerformImplicitConversion() at SemaExprCXX.cpp:2,964 0x14567124	
clang::InitializationSequence::Perform() at SemaInit.cpp:6,315 0x1465e444	
TryStaticImplicitCast() at SemaCast.cpp:1,522 0x141bb004	
TryStaticCast() at SemaCast.cpp:988 0x141b9e98	
(anonymous namespace)::CastOperation::CheckCXXCStyleCast at SemaCast.cpp:2,130 0x141b4248	
clang::Sema::BuildCStyleCastExpr() at SemaCast.cpp:2,455 0x141b3b60	
clang::Sema::ActOnCastExpr() at SemaExpr.cpp:5,545 0x144533a8

I see CK_NoOp is hardcoded in the caller:

case ICK_Qualification: {
  // The qualification keeps the category of the inner expression, unless the
  // target type isn't a reference.
  ExprValueKind VK = ToType->isReferenceType() ?
                                From->getValueKind() : VK_RValue;
  From = ImpCastExprToType(From, ToType.getNonLValueExprType(Context),
                           CK_NoOp, VK, /*BasePath=*/nullptr, CCK).get();

Should I fix something in here?

389–395

Got it. Moved the code up.

test/Sema/address-space-cast.c

1–2 ↗

(On Diff #25808)

Ok, doing now the proper checks!

[rsmith]

lib/Sema/Sema.cpp

343

If that's the only code path through which we get this wrong, my preference would be to move the fix into PerformImplicitConversion, and add an assert to ImpCastExprToType that we're not changing the address space with a CK_NoOp cast.

test/Sema/address-space-cast.c

1 ↗

(On Diff #27420)

Please move this test to test/CodeGen (I know, you're testing a Sema change, but the primary impact of your change is to make us emit the right IR, and that's what you're testing.)

[sfantao]

Moved the fix up the stack and changed the location of the regression test.

lib/Sema/Sema.cpp

343

I was comparing what was going on for C and C++ and I see that for C the address cast is identified right away in CheckCStyleCast in:

Kind = Self.PrepareScalarCast(SrcExpr, DestType);

Therefore I thought it would make sense to do the same for C++ in CheckCXXCStyleCast. I added the assertion in the place you suggested anyway.

I understand this is not exactly what you requested but it looks as a more clean fix. If you do not agree, let me know and I can switch back to what you were originally proposing.

test/Sema/address-space-cast.c

1 ↗

(On Diff #27420)

Done!

[sfantao]

Any more comments about this patch?

Thanks!
Samuel

[sfantao]

Ping!

[sfantao]

Ping!

[sfantao]

Ping!

[rsmith]

lib/Sema/Sema.cpp

366

Typo "no" -> "not"

lib/Sema/SemaCast.cpp

2106–2113

Looks like this would allow casting between any pointer type and any other pointer type with a different address space, independent of the pointee type. That's not right; we should still diagnose casts between pointer-to-object and pointer-to-function types and the like. Also, reinterpret_casts will still fail with this check. This check should go at the end of TryReinterpretCast rather than here.

[sfantao]

Fix typo.

[sfantao]

Thanks for the review!

Please see the inline comment below.

Thanks again,
Samuel

lib/Sema/Sema.cpp

366

Done!

lib/Sema/SemaCast.cpp

2106–2113

I agree, the pointee type should also be checked.

However, if I move the check to TryReinterpretCast a static cast will be attempted first and will hit the assertion I've just added to Sema.cpp. Do you suggest to add something to TryStaticCast as well? Or should I just add the pointee check?

[sfantao]

Rebase and add check to make sure the pointee of the pointers being casted match.

In a previous review, it was suggested by Richard Smith to move the check to the end of TryReinterpretCast. However, that does not solve the issue given that it is possible to have a successful cast try before the end of TryReinterpretCast is reached. The testcase in this patch is one of those cases.

[sfantao]

Ping!

[sfantao]

• Rebase.

[sfantao]

Any more comments on this patch?

Thanks!
Samuel

[sfantao]

Rebase.

lib/Sema/Sema.cpp

void Sema::PrintStats() const {
   BumpAlloc.PrintStats();
   AnalysisWarnings.PrintStats();
 }
 
 /// ImpCastExprToType - If Expr is not of type 'Type', insert an implicit cast.
 /// If there is already an implicit cast, merge into the existing one.
 /// The result is of the given category.
 ExprResult Sema::ImpCastExprToType(Expr *E, QualType Ty,
                                    CastKind Kind, ExprValueKind VK,
                                    const CXXCastPath *BasePath,
                                    CheckedConversionKind CCK) {
 #ifndef NDEBUG
   if (VK == VK_RValue && !E->isRValue()) {
     switch (Kind) {
     default:
       llvm_unreachable("can't implicitly cast lvalue to rvalue with this cast "
                        "kind");
     case CK_LValueToRValue:
     case CK_ArrayToPointerDecay:
     case CK_FunctionToPointerDecay:
     case CK_ToVoid:
       break;
     }
   }
   assert((VK == VK_RValue || !E->isRValue()) && "can't cast rvalue to lvalue");
 #endif
 
   // Check whether we're implicitly casting from a nullable type to a nonnull
   // type.
   if (auto exprNullability = E->getType()->getNullability(Context)) {
     if (*exprNullability == NullabilityKind::Nullable) {
       if (auto typeNullability = Ty->getNullability(Context)) {
         if (*typeNullability == NullabilityKind::NonNull) {
           Diag(E->getLocStart(), diag::warn_nullability_lost)
             << E->getType() << Ty;
         }
       }
     }
   }
 
   QualType ExprTy = Context.getCanonicalType(E->getType());
   QualType TypeTy = Context.getCanonicalType(Ty);
 
   if (ExprTy == TypeTy)
     return E;
 
+  // In the event an address space cast is requested, the kind passed from the
+  // caller should not be CK_NoOp.
+  assert((Kind != CK_NoOp ||
+          !(ExprTy->isPointerType() && TypeTy->isPointerType() &&
+            ExprTy->getPointeeType().getAddressSpace() !=
+                TypeTy->getPointeeType().getAddressSpace())) &&
+         "NoOp is not a valid kind for and address cast");
+
   if (ImplicitCastExpr *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
     if (ImpCast->getCastKind() == Kind && (!BasePath || BasePath->empty())) {
       ImpCast->setType(Ty);
       ImpCast->setValueKind(VK);
       return E;
     }
   }
 
   return ImplicitCastExpr::Create(Context, Ty, Kind, E, BasePath, VK);
 }
 
 /// ScalarTypeToBooleanCastKind - Returns the cast kind corresponding
 /// to the conversion from scalar type ScalarTy to the Boolean type.
 CastKind Sema::ScalarTypeToBooleanCastKind(QualType ScalarTy) {
   switch (ScalarTy->getScalarTypeKind()) {
   case Type::STK_Bool: return CK_NoOp;
   case Type::STK_CPointer: return CK_PointerToBoolean;
   case Type::STK_BlockPointer: return CK_PointerToBoolean;
   case Type::STK_ObjCObjectPointer: return CK_PointerToBoolean;

lib/Sema/SemaCast.cpp

void CastOperation::CheckCXXCStyleCast(bool FunctionalStyle,
 
   // AltiVec vector initialization with a single literal.
   if (const VectorType *vecTy = DestType->getAs<VectorType>())
     if (vecTy->getVectorKind() == VectorType::AltiVecVector
         && (SrcExpr.get()->getType()->isIntegerType()
             || SrcExpr.get()->getType()->isFloatingType())) {
       Kind = CK_VectorSplat;
       return;
     }
 
+  // If we are casting pointers, we need to check whether this refers to an
+  // address cast.
+  if (DestType->isPointerType() && SrcExpr.get()->getType()->isPointerType() &&
+      DestType->getPointeeType().getAddressSpace() !=
+          SrcExpr.get()->getType().getAddressSpace()) {
+    Kind = CK_AddressSpaceConversion;
+    return;
+  }
+
   // C++ [expr.cast]p5: The conversions performed by
   //   - a const_cast,
   //   - a static_cast,
   //   - a static_cast followed by a const_cast,
   //   - a reinterpret_cast, or
   //   - a reinterpret_cast followed by a const_cast,
   //   can be performed using the cast notation of explicit type conversion.
   //   [...] If a conversion can be interpreted in more than one of the ways

test/CodeGen/address-space-explicit-cast.c

+// RUN: %clang_cc1 -emit-llvm -o - -x c %s | FileCheck -check-prefix=CHECK %s
+// RUN: %clang_cc1 -emit-llvm -o - -x c++ %s | FileCheck -check-prefix=CHECK-CXX %s
+
+typedef __attribute__((address_space(1))) char *AddrSpaceCharType;
+
+// CHECK-LABEL: @foo()
+// CHECK-CXX-LABEL: @_Z3foov()
+void foo() {
+  // CHECK: %p = alloca i8 addrspace(1)*
+  // CHECK-CXX: %p = alloca i8 addrspace(1)*
+  AddrSpaceCharType p;
+
+  // CHECK: store i8 addrspace(1)* addrspacecast ({{.*}} to i8 addrspace(1)*), i8 addrspace(1)** %p
+  // CHECK-CXX: store i8 addrspace(1)* addrspacecast ({{.*}} to i8 addrspace(1)*), i8 addrspace(1)** %p
+  p = (AddrSpaceCharType) "a";
+}