[Sema] Mark implicitly-inserted ICE's as being part of explicit cast (PR38166)

[lebedev.ri]

Summary

As discussed in PR38166, we need to be able to distinqush whether the cast
we are visiting is actually a cast, or part of an ExplicitCast.
There are at least four ways to get there:

1. Introduce a new CastKind, and use it instead of IntegralCast if we are in ExplicitCast.
   
   Would work, but does not scale - what if we will need more of these cast kinds?
2. Introduce a flag in CastExprBits, whether this cast is part of ExplicitCast or not.
   
   Would work, but it isn't immediately clear where it needs to be set.
3. Fix ScalarExprEmitter::VisitCastExpr() to visit these NoOp casts.
   
   As pointed out by @rsmith, CodeGenFunction::EmitMaterializeTemporaryExpr calls
   
   skipRValueSubobjectAdjustments, which steps over the CK_NoOp cast`,
   
   which explains why we currently don't visit those.
   
   This is probably impossible, as @efriedma points out, that is intentional as per [class.temporary] in the standard
4. And the simplest one, just record which NoOp casts we skip.
   
   It just kinda works as-is afterwards.

But, the approach with a flag is the least intrusive one, and is probably the best one overall.

[efriedma]

skipRValueSubobjectAdjustments has to match the rules in [class.temporary] in the standard, which includes skipping over certain explicit casts.

Would it be enough to accumulate the skipped casts into a SmallVector, like we do for the skipped comma operators?

test/CodeGenCXX/const-init-cxx11.cpp

229 ↗

(On Diff #156137)

This change is suspect; note the comment "this creates a non-const temporary".

[rjmccall]

I think it would be reasonable to set a flag on ImplicitCastExprs that are actually semantically part of an explicit cast. I don't think that would be hard to get Sema to do, either by passing a flag down to the code that builds those casts or just by retroactively setting that flag on all the ICE sub-expressions of an explicit cast when "capping" it with the ExplicitCastExpr.

[lebedev.ri]

In D49508#1167177, @efriedma wrote:

skipRValueSubobjectAdjustments has to match the rules in [class.temporary] in the standard, which includes skipping over certain explicit casts.

I'm this approach because this is what @rsmith suggested. As i said, i don't really know this code,
so maybe i'm missing something obvious that makes this still possible.

Would it be enough to accumulate the skipped casts into a SmallVector, like we do for the skipped comma operators?

Hmm, I'm not sure. It depends whether we return from CodeGenFunction::EmitMaterializeTemporaryExpr(),
or are still within the CodeGenFunction::EmitMaterializeTemporaryExpr() function, when visiting nested nodes.
This might work, but let's see if i can make the flag approach work.

In D49508#1167566, @rjmccall wrote:

I think it would be reasonable to set a flag on ImplicitCastExprs that are actually semantically part of an explicit cast. I don't think that would be hard to get Sema to do, either by passing a flag down to the code that builds those casts or just by retroactively setting that flag on all the ICE sub-expressions of an explicit cast when "capping" it with the ExplicitCastExpr.

That was my initial thought, too.
I wasn't sure how to approach this though.

[lebedev.ri]

In D49508#1167177, @efriedma wrote:

Would it be enough to accumulate the skipped casts into a SmallVector, like we do for the skipped comma operators?

Duuuh, why did i not try this in the first place?!
That works, thanks!

[rjmccall]

Hmm. I think the approach of flagging ICEs that are semantically part of an explicit cast is probably a better representation for tools across the board.

If we *are* going to do it this way, though, I think you should (1) make the collection of skipped expressions optional and (2) collect *all* the skipped expressions, not just no-op casts.

[lebedev.ri]

In D49508#1168584, @rjmccall wrote:

Hmm. I think the approach of flagging ICEs that are semantically part of an explicit cast is probably a better representation for tools across the board.

I could do that, but i couldn't find where it should be done.
Where are these "ICEs that are semantically part of an explicit cast" created? Where would we mark them?

If we *are* going to do it this way, though, I think you should (1) make the collection of skipped expressions optional and (2) collect *all* the skipped expressions, not just no-op casts.

1. I was wondering about that, will do.
2. Well, i could do that, but i would need to filter them afterwards in my use-case. So i wonder - what is the motivation for that? Nothing else needs that additional knowledge right now.

[rjmccall]

In D49508#1168599, @lebedev.ri wrote:

In D49508#1168584, @rjmccall wrote:

Hmm. I think the approach of flagging ICEs that are semantically part of an explicit cast is probably a better representation for tools across the board.

I could do that, but i couldn't find where it should be done.
Where are these "ICEs that are semantically part of an explicit cast" created?

The design of CastExpr is that each node does exactly one implicit conversion step, but sometimes a conversion involves performing multiple steps which each has to get its own node, so all those initial conversions have to be represented as ICEs. Also, while we do make some effort to give the explicit cast node a meaningful CastKind, in some situations it's just easier to build an ICE with the real CK and then just make the explicit cast a NoOp. All those ICEs are triggered by the checking of the explicit cast and therefore semantically part of that operation even if they're separate nodes for technical reasons.

Where would we mark them?

Well, explicit casts are handled by SemaCast.cpp, and all the checking routines are managed by a CastOperation helper class. In fact, it's set up so that you could very easily (1) stash the original expression in the CastOperation construct and then (2) in complete(), just walk from the given cast up to the original expression and mark every ICE you see as being part of the explicit cast.

If we *are* going to do it this way, though, I think you should (1) make the collection of skipped expressions optional and (2) collect *all* the skipped expressions, not just no-op casts.

1. I was wondering about that, will do.
2. Well, i could do that, but i would need to filter them afterwards in my use-case.

Well, you're filtering them anyway, right? You're scanning the list for explicit casts.

So i wonder - what is the motivation for that?
Nothing else needs that additional knowledge right now.

It has a much more obvious specification and it's not hard to imagine clients that would want to scan that list for other things. (In fact, the comma-expressions list could pretty easily be combined into that.)

[lebedev.ri]

In D49508#1168620, @rjmccall wrote:

In D49508#1168599, @lebedev.ri wrote:

In D49508#1168584, @rjmccall wrote:

Hmm. I think the approach of flagging ICEs that are semantically part of an explicit cast is probably a better representation for tools across the board.

I could do that, but i couldn't find where it should be done.
Where are these "ICEs that are semantically part of an explicit cast" created?

The design of CastExpr is that each node does exactly one implicit conversion step, but sometimes a conversion involves performing multiple steps which each has to get its own node, so all those initial conversions have to be represented as ICEs. Also, while we do make some effort to give the explicit cast node a meaningful CastKind, in some situations it's just easier to build an ICE with the real CK and then just make the explicit cast a NoOp. All those ICEs are triggered by the checking of the explicit cast and therefore semantically part of that operation even if they're separate nodes for technical reasons.

Where would we mark them?

Well, explicit casts are handled by SemaCast.cpp, and all the checking routines are managed by a CastOperation helper class. In fact, it's set up so that you could very easily (1) stash the original expression in the CastOperation construct and then (2) in complete(), just walk from the given cast up to the original expression and mark every ICE you see as being part of the explicit cast.

Hmm, thank you for these pointers. I agree that it would be *nicer*. Let's see..

If we *are* going to do it this way, though, I think you should (1) make the collection of skipped expressions optional and (2) collect *all* the skipped expressions, not just no-op casts.

1. I was wondering about that, will do.
2. Well, i could do that, but i would need to filter them afterwards in my use-case.

Well, you're filtering them anyway, right? You're scanning the list for explicit casts.

I meant filter the list to only contain skipped casts, before actually scanning it for specific conditions.

So i wonder - what is the motivation for that?
Nothing else needs that additional knowledge right now.

It has a much more obvious specification and it's not hard to imagine clients that would want to scan that list for other things. (In fact, the comma-expressions list could pretty easily be combined into that.)

[lebedev.ri]

Moved to the flag approach, which was my initial thought.
This ended up being rather too simple, of all the alternative solutions.
Oh well, that was rather sad waste of time.

[rjmccall]

include/clang/AST/Stmt.h

205 ↗

(On Diff #156465)

This needs to be unsigned to pack optimally on MSVC.

206 ↗

(On Diff #156465)

This needs to be serialized.

[lebedev.ri]

include/clang/AST/Stmt.h

206 ↗

(On Diff #156465)

Uhm, could you please explain what do you mean by 'serialized'?

[rjmccall]

include/clang/AST/Stmt.h

206 ↗

(On Diff #156465)

It needs to be preserved when writing an ICE into a PCH / module file. See the ASTWriter / ASTReader.

[lebedev.ri]

• Use unsigned, not bool.
• Serialization, although without tests, and likely incompatible with previous versions.

include/clang/AST/Stmt.h

206 ↗

(On Diff #156465)

Aha. I did add handling there but it raises questions:

1. This will silently break with different AST serialization versions. I'm not sure how to handle it, since VERSION_MINOR isn't even read back.
2. Does this need a test? How to write one? Like ./test/PCH/include-timestamp.cpp, using llvm-bcanalyzer?

[rsmith]

include/clang/AST/Stmt.h

206 ↗

(On Diff #156465)

Don't worry about breaking the serialization format. We do not maintain AST file format compatibility in general (neither between major releases nor between any two arbitrary SVN revisions). [We should probably maintain file format compatibility between patch releases, but I don't think that works right now because we check the full version number including the patch level when reading a file.]

Please do add a test: what you need to do is create a PCH containing an implicit cast expression and then import that AST file and do anything to check that the value is preserved (such as inspecting the output of -ast-dump). Maybe you could add this to the existing test/PCH/cxx_exprs.cpp test, which already does most of what you want, but doesn't have any FileCheck tests on the -ast-dump output yet.

lib/AST/ASTDumper.cpp

2122 ↗

(On Diff #156568)

Our predominant convention is to use lower_snake_case for such things in -ast-dump (though we're not exactly consistent on this...)

lib/Sema/SemaCast.cpp

94–101 ↗

(On Diff #156568)

You don't need to track the OrigSrcExpr here. You can just recurse down through all the ImplicitCastExprs (they're always all notionally part of the explicit cast).

[lebedev.ri]

lib/Sema/SemaCast.cpp

94–101 ↗

(On Diff #156568)

We do sometimes have OrigSrcExpr being ImplicitCastExpr.
https://godbolt.org/g/S5951G <- that ImplicitCastExpr would now get marked, even though it is OrigSrcExpr.
Is that expected?

[lebedev.ri]

include/clang/AST/Stmt.h

206 ↗

(On Diff #156465)

Uhm, so i have tried to add a test.
With git master, the -ast-dump from the first runline of test/PCH/cxx_exprs.cpp contains:

|-TypedefDecl 0x55a348020b88 </build/clang/test/PCH/cxx_exprs.h:5:1, col:44> col:44 referenced static_cast_result 'typeof (static_cast<void *>(0))':'void *'
| `-TypeOfExprType 0x55a348020b50 'typeof (static_cast<void *>(0))' sugar
|   |-ParenExpr 0x55a348020b28 <col:19, col:42> 'void *'
|   | `-CXXStaticCastExpr 0x55a348020af8 <col:20, col:41> 'void *' static_cast<void *> <NoOp>
|   |   `-ImplicitCastExpr 0x55a348020ae0 <col:40> 'void *' <NullToPointer>
|   |     `-IntegerLiteral 0x55a348020aa8 <col:40> 'int' 0
|   `-PointerType 0x55a347fe6390 'void *'
|     `-BuiltinType 0x55a347fe5b90 'void'

But the last runline (which uses PCH) does not have that.
Unfortunately i know next to nothing about PCH / AST serialization, so i'm not sure how to workaround that yet.
It does contain `-<undeserialized declarations>, so maybe i have to somehow force full deserialization?

cxx_exprs.cpp.tmp.dump024 KBDownload

cxx_exprs.cpp.tmp.dump15 KBDownload

[lebedev.ri]

Partially address some of @rsmith's review notes, see reply notes for issues.

[lebedev.ri]

Hurray, got the PCH test working!
I'm still unsure about marking *all* the immediate implicit casts as part of the group.

[rsmith]

lib/Sema/SemaCast.cpp

94–101 ↗

(On Diff #156568)

Yes, that's expected, the conversion from int to char there is part of the semantics of the explicit cast from int to vector of char, and you wouldn't want your UBSan check to warn about that, right?

[lebedev.ri]

Thank you for the review!

lib/Sema/SemaCast.cpp

94–101 ↗

(On Diff #156568)

I was just surprized since that ImplicitCastExpr was there before this CastExpr handling.
If i make that splat implicit, it now warns about the truncation, https://godbolt.org/g/Ax7UTw
So yeah, i guess this is correct.

include/clang/AST/Expr.h

public:
   /// for the object itself.
   const Expr *getBestDynamicClassTypeExpr() const;
 
   /// Walk outwards from an expression we want to bind a reference to and
   /// find the expression whose lifetime needs to be extended. Record
   /// the LHSs of comma expressions and adjustments needed along the path.
   const Expr *skipRValueSubobjectAdjustments(
       SmallVectorImpl<const Expr *> &CommaLHS,
-      SmallVectorImpl<SubobjectAdjustment> &Adjustments) const;
+      SmallVectorImpl<SubobjectAdjustment> &Adjustments,
+      SmallVectorImpl<const CastExpr *> &SkippedCasts) const;
   const Expr *skipRValueSubobjectAdjustments() const {
     SmallVector<const Expr *, 8> CommaLHSs;
     SmallVector<SubobjectAdjustment, 8> Adjustments;
-    return skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+    SmallVector<const CastExpr *, 2> SkippedCasts;
+    return skipRValueSubobjectAdjustments(CommaLHSs, Adjustments, SkippedCasts);
   }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() >= firstExprConstant &&
            T->getStmtClass() <= lastExprConstant;
   }
 };
 

lib/AST/Expr.cpp

const CXXRecordDecl *Expr::getBestDynamicClassType() const {
 
   const RecordType *Ty = DerivedType->castAs<RecordType>();
   Decl *D = Ty->getDecl();
   return cast<CXXRecordDecl>(D);
 }
 
 const Expr *Expr::skipRValueSubobjectAdjustments(
     SmallVectorImpl<const Expr *> &CommaLHSs,
-    SmallVectorImpl<SubobjectAdjustment> &Adjustments) const {
+    SmallVectorImpl<SubobjectAdjustment> &Adjustments,
+    SmallVectorImpl<const CastExpr *> &SkippedCasts) const {
   const Expr *E = this;
   while (true) {
     E = E->IgnoreParens();
 
     if (const CastExpr *CE = dyn_cast<CastExpr>(E)) {
       if ((CE->getCastKind() == CK_DerivedToBase ||
            CE->getCastKind() == CK_UncheckedDerivedToBase) &&
           E->getType()->isRecordType()) {
         E = CE->getSubExpr();
         CXXRecordDecl *Derived
           = cast<CXXRecordDecl>(E->getType()->getAs<RecordType>()->getDecl());
         Adjustments.push_back(SubobjectAdjustment(CE, Derived));
         continue;
       }
 
       if (CE->getCastKind() == CK_NoOp) {
+        SkippedCasts.emplace_back(CE);
         E = CE->getSubExpr();
         continue;
       }
     } else if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
       if (!ME->isArrow()) {
         assert(ME->getBase()->getType()->isRecordType());
         if (FieldDecl *Field = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
           if (!Field->isBitField() && !Field->getType()->isReferenceType()) {

lib/AST/ExprConstant.cpp

static QualType getType(APValue::LValueBase B) {
     const Expr *Base = B.get<const Expr*>();
 
     // For a materialized temporary, the type of the temporary we materialized
     // may not be the type of the expression.
     if (const MaterializeTemporaryExpr *MTE =
             dyn_cast<MaterializeTemporaryExpr>(Base)) {
       SmallVector<const Expr *, 2> CommaLHSs;
       SmallVector<SubobjectAdjustment, 2> Adjustments;
+      SmallVector<const CastExpr *, 2> SkippedCasts;
       const Expr *Temp = MTE->GetTemporaryExpr();
-      const Expr *Inner = Temp->skipRValueSubobjectAdjustments(CommaLHSs,
-                                                               Adjustments);
+      const Expr *Inner = Temp->skipRValueSubobjectAdjustments(
+          CommaLHSs, Adjustments, SkippedCasts);
       // Keep any cv-qualifiers from the reference if we generated a temporary
       // for it directly. Otherwise use the type after adjustment.
       if (!Adjustments.empty())
         return Inner->getType();
     }
 
     return Base->getType();
   }
bool LValueExprEvaluator::VisitVarDecl(const Expr *E, const VarDecl *VD) {
   return Success(*V, E);
 }
 
 bool LValueExprEvaluator::VisitMaterializeTemporaryExpr(
     const MaterializeTemporaryExpr *E) {
   // Walk through the expression to find the materialized temporary itself.
   SmallVector<const Expr *, 2> CommaLHSs;
   SmallVector<SubobjectAdjustment, 2> Adjustments;
-  const Expr *Inner = E->GetTemporaryExpr()->
-      skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+  SmallVector<const CastExpr *, 2> SkippedCasts;
+  const Expr *Inner = E->GetTemporaryExpr()->skipRValueSubobjectAdjustments(
+      CommaLHSs, Adjustments, SkippedCasts);
 
   // If we passed any comma operators, evaluate their LHSs.
   for (unsigned I = 0, N = CommaLHSs.size(); I != N; ++I)
     if (!EvaluateIgnoredValue(Info, CommaLHSs[I]))
       return false;
 
   // A materialized temporary with static storage duration can appear within the
   // result of a constant expression evaluation, so we need to preserve its

lib/Analysis/CFG.cpp

if (const MaterializeTemporaryExpr *MTE
       if (FoundMTE)
         *FoundMTE = true;
       continue;
     }
 
     // Skip sub-object accesses into rvalues.
     SmallVector<const Expr *, 2> CommaLHSs;
     SmallVector<SubobjectAdjustment, 2> Adjustments;
-    const Expr *SkippedInit =
-        Init->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+    SmallVector<const CastExpr *, 2> SkippedCasts;
+    const Expr *SkippedInit = Init->skipRValueSubobjectAdjustments(
+        CommaLHSs, Adjustments, SkippedCasts);
     if (SkippedInit != Init) {
       Init = SkippedInit;
       continue;
     }
 
     break;
   }
 
case Stmt::ParenExprClass:
       E = cast<ParenExpr>(E)->getSubExpr();
       goto tryAgain;
 
     case Stmt::MaterializeTemporaryExprClass: {
       const MaterializeTemporaryExpr* MTE = cast<MaterializeTemporaryExpr>(E);
       BindToTemporary = (MTE->getStorageDuration() != SD_FullExpression);
       SmallVector<const Expr *, 2> CommaLHSs;
       SmallVector<SubobjectAdjustment, 2> Adjustments;
+      SmallVector<const CastExpr *, 2> SkippedCasts;
       // Find the expression whose lifetime needs to be extended.
-      E = const_cast<Expr *>(
-          cast<MaterializeTemporaryExpr>(E)
+      E = const_cast<Expr *>(cast<MaterializeTemporaryExpr>(E)
                                  ->GetTemporaryExpr()
-              ->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments));
+                                 ->skipRValueSubobjectAdjustments(
+                                     CommaLHSs, Adjustments, SkippedCasts));
       // Visit the skipped comma operator left-hand sides for other temporaries.
       for (const Expr *CommaLHS : CommaLHSs) {
         VisitForTemporaryDtors(const_cast<Expr *>(CommaLHS),
                                /*BindToTemporary=*/false, Context);
       }
       goto tryAgain;
     }
 

lib/CodeGen/CGExpr.cpp

if (ownership != Qualifiers::OCL_None &&
     }
 
     pushTemporaryCleanup(*this, M, E, Object);
     return RefTempDst;
   }
 
   SmallVector<const Expr *, 2> CommaLHSs;
   SmallVector<SubobjectAdjustment, 2> Adjustments;
-  E = E->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+  SmallVector<const CastExpr *, 2> SkippedCasts;
+  E = E->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments, SkippedCasts);
 
   for (const auto &Ignored : CommaLHSs)
     EmitIgnoredExpr(Ignored);
 
   if (const auto *opaque = dyn_cast<OpaqueValueExpr>(E)) {
     if (opaque->getType()->isRecordType()) {
       assert(Adjustments.empty());
       return EmitOpaqueValueLValue(opaque);

lib/CodeGen/CGExprConstant.cpp

ConstantLValueEmitter::VisitCXXTypeidExpr(const CXXTypeidExpr *E) {
   return CGM.GetAddrOfRTTIDescriptor(T);
 }
 
 ConstantLValue
 ConstantLValueEmitter::VisitCXXUuidofExpr(const CXXUuidofExpr *E) {
   return CGM.GetAddrOfUuidDescriptor(E);
 }
 
-ConstantLValue
-ConstantLValueEmitter::VisitMaterializeTemporaryExpr(
-                                            const MaterializeTemporaryExpr *E) {
+ConstantLValue ConstantLValueEmitter::VisitMaterializeTemporaryExpr(
   assert(E->getStorageDuration() == SD_Static);
   SmallVector<const Expr *, 2> CommaLHSs;
   SmallVector<SubobjectAdjustment, 2> Adjustments;
-  const Expr *Inner = E->GetTemporaryExpr()
-      ->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+  SmallVector<const CastExpr *, 2> SkippedCasts;
+  const Expr *Inner = E->GetTemporaryExpr()->skipRValueSubobjectAdjustments(
+      CommaLHSs, Adjustments, SkippedCasts);
   return CGM.GetAddrOfGlobalTemporary(E, Inner);
 }
 
 llvm::Constant *ConstantEmitter::tryEmitPrivate(const APValue &Value,
                                                 QualType DestType) {
   switch (Value.getKind()) {
   case APValue::Uninitialized:
     llvm_unreachable("Constant expressions should be initialized.");

lib/Sema/JumpDiagnostics.cpp

void JumpScopeChecker::BuildScopeInformation(Stmt *S,
 
   case Stmt::MaterializeTemporaryExprClass: {
     // Disallow jumps out of scopes containing temporaries lifetime-extended to
     // automatic storage duration.
     MaterializeTemporaryExpr *MTE = cast<MaterializeTemporaryExpr>(S);
     if (MTE->getStorageDuration() == SD_Automatic) {
       SmallVector<const Expr *, 4> CommaLHS;
       SmallVector<SubobjectAdjustment, 4> Adjustments;
+      SmallVector<const CastExpr *, 2> SkippedCasts;
       const Expr *ExtendedObject =
           MTE->GetTemporaryExpr()->skipRValueSubobjectAdjustments(
-              CommaLHS, Adjustments);
+              CommaLHS, Adjustments, SkippedCasts);
       if (ExtendedObject->getType().isDestructedType()) {
         Scopes.push_back(GotoScope(ParentScope, 0,
                                    diag::note_exits_temporary_dtor,
                                    ExtendedObject->getExprLoc()));
         origParentScope = Scopes.size()-1;
       }
     }
     break;

lib/StaticAnalyzer/Core/ExprEngine.cpp

ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
   //
   // Use the usual methods for obtaining the expression of the base object,
   // and record the adjustments that we need to make to obtain the sub-object
   // that the whole expression 'Ex' refers to. This trick is usual,
   // in the sense that CodeGen takes a similar route.
 
   SmallVector<const Expr *, 2> CommaLHSs;
   SmallVector<SubobjectAdjustment, 2> Adjustments;
+  SmallVector<const CastExpr *, 2> SkippedCasts;
 
   const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments(
-      CommaLHSs, Adjustments);
+      CommaLHSs, Adjustments, SkippedCasts);
 
   // Take the region for Init, i.e. for the whole object. If we do not remember
   // the region in which the object originally was constructed, come up with
   // a new temporary region out of thin air and copy the contents of the object
   // (which are currently present in the Environment, because Init is an rvalue)
   // into that region. This is not correct, but it is better than nothing.
   const TypedValueRegion *TR = nullptr;
   if (const auto *MT = dyn_cast<MaterializeTemporaryExpr>(Result)) {