This is an archive of the discontinued LLVM Phabricator instance.

Fix adress cast for C++ in SEMA
Needs ReviewPublic

Authored by sfantao on Feb 12 2015, 9:57 PM.

Details

Reviewers
rsmith
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.

Diff Detail

Event Timeline

sfantao updated this revision to Diff 19872.Feb 12 2015, 9:57 PM
sfantao retitled this revision from to Fix adress cast for C++ in SEMA.
sfantao updated this object.
sfantao edited the test plan for this revision. (Show Details)
sfantao added a subscriber: Unknown Object (MLST).
hfinkel added inline comments.
lib/Sema/Sema.cpp
412

Add a period to the end of the comment.

413

Does this apply to CK_LValueBitCast too?

sfantao updated this revision to Diff 25708.May 13 2015, 10:23 AM

Add period to comment.

Hi Hal,

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

Regards,
Samuel

lib/Sema/Sema.cpp
412

Will do!

413

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.

Also, this needs a test case.

sfantao updated this revision to Diff 25808.May 14 2015, 1:59 PM

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

Any other comments on this patch?

Thanks!
Samuel

rsmith added inline comments.Jun 8 2015, 11:17 AM
lib/Sema/Sema.cpp
357

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?

403–409

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 updated this revision to Diff 27420.Jun 9 2015, 5:28 PM

Refactor the patch following rsmith requests.

sfantao added inline comments.Jun 9 2015, 5:31 PM
lib/Sema/Sema.cpp
357

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?

403–409

Got it. Moved the code up.

test/Sema/address-space-cast.c
1–2 ↗(On Diff #25808)

Ok, doing now the proper checks!

rsmith added inline comments.Jun 9 2015, 5:55 PM
lib/Sema/Sema.cpp
357

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 updated this revision to Diff 27466.Jun 10 2015, 2:43 PM

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

lib/Sema/Sema.cpp
357

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!

Any more comments about this patch?

Thanks!
Samuel

rsmith added inline comments.Jul 21 2015, 2:11 PM
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 updated this revision to Diff 30558.Jul 23 2015, 8:51 PM

Fix typo.

Thanks for the review!

Please see the inline comment below.

Thanks again,
Samuel

lib/Sema/Sema.cpp
380

Done!

lib/Sema/SemaCast.cpp
2105–2123

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 updated this revision to Diff 39619.Nov 6 2015, 6:44 PM

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 updated this revision to Diff 41676.Dec 2 2015, 2:09 PM
  • Rebase.

Any more comments on this patch?

Thanks!
Samuel

sfantao updated this revision to Diff 44124.Jan 6 2016, 7:59 AM

Rebase.