llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: None (martinboehme) <details> <summary>Changes</summary> This is a relatively rare case, but - It's still nice to get this right, - We can remove the special case for this in `VisitCXXOperatorCallExpr()` (that simply bails out), and - With this in place, I can avoid having to add a similar special case in an upcoming patch. --- Full diff: https://github.com/llvm/llvm-project/pull/85064.diff 5 Files Affected: - (modified) clang/include/clang/Analysis/FlowSensitive/RecordOps.h (+5-1) - (modified) clang/lib/Analysis/FlowSensitive/RecordOps.cpp (+56-38) - (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (-9) - (modified) clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp (+44-2) - (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+21-4) ``````````diff diff --git a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h index 783e53e980aa2c..8fad45fc11d81e 100644 --- a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h +++ b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h @@ -31,7 +31,11 @@ namespace dataflow { /// /// Requirements: /// -/// `Src` and `Dst` must have the same canonical unqualified type. +/// Either: +/// - `Src` and `Dest` must have the same canonical unqualified type, or +/// - The type of `Src` must be derived from `Dest`, or +/// - The type of `Dest` must be derived from `Src` (in this case, any fields +/// that are only present in `Dest` are not overwritten). void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst, Environment &Env); diff --git a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp index da4dd6dc078515..4fc4c15a07a1ce 100644 --- a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp +++ b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp @@ -14,18 +14,52 @@ #define DEBUG_TYPE "dataflow" -void clang::dataflow::copyRecord(RecordStorageLocation &Src, - RecordStorageLocation &Dst, Environment &Env) { +namespace clang::dataflow { + +static void copyField(const ValueDecl *Field, StorageLocation *SrcFieldLoc, + StorageLocation *DstFieldLoc, RecordStorageLocation &Dst, + Environment &Env) { + assert(Field->getType()->isReferenceType() || + (SrcFieldLoc != nullptr && DstFieldLoc != nullptr)); + + if (Field->getType()->isRecordType()) { + copyRecord(cast<RecordStorageLocation>(*SrcFieldLoc), + cast<RecordStorageLocation>(*DstFieldLoc), Env); + } else if (Field->getType()->isReferenceType()) { + Dst.setChild(*Field, SrcFieldLoc); + } else { + if (Value *Val = Env.getValue(*SrcFieldLoc)) + Env.setValue(*DstFieldLoc, *Val); + else + Env.clearValue(*DstFieldLoc); + } +} + +static void copySyntheticField(QualType FieldType, StorageLocation &SrcFieldLoc, + StorageLocation &DstFieldLoc, Environment &Env) { + if (FieldType->isRecordType()) { + copyRecord(cast<RecordStorageLocation>(SrcFieldLoc), + cast<RecordStorageLocation>(DstFieldLoc), Env); + } else { + if (Value *Val = Env.getValue(SrcFieldLoc)) + Env.setValue(DstFieldLoc, *Val); + else + Env.clearValue(DstFieldLoc); + } +} + +void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst, + Environment &Env) { auto SrcType = Src.getType().getCanonicalType().getUnqualifiedType(); auto DstType = Dst.getType().getCanonicalType().getUnqualifiedType(); auto SrcDecl = SrcType->getAsCXXRecordDecl(); auto DstDecl = DstType->getAsCXXRecordDecl(); - bool compatibleTypes = + [[maybe_unused]] bool compatibleTypes = SrcType == DstType || - (SrcDecl && DstDecl && SrcDecl->isDerivedFrom(DstDecl)); - (void)compatibleTypes; + (SrcDecl != nullptr && DstDecl != nullptr && + (SrcDecl->isDerivedFrom(DstDecl) || DstDecl->isDerivedFrom(SrcDecl))); LLVM_DEBUG({ if (!compatibleTypes) { @@ -35,45 +69,27 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src, }); assert(compatibleTypes); - for (auto [Field, DstFieldLoc] : Dst.children()) { - StorageLocation *SrcFieldLoc = Src.getChild(*Field); - - assert(Field->getType()->isReferenceType() || - (SrcFieldLoc != nullptr && DstFieldLoc != nullptr)); - - if (Field->getType()->isRecordType()) { - copyRecord(cast<RecordStorageLocation>(*SrcFieldLoc), - cast<RecordStorageLocation>(*DstFieldLoc), Env); - } else if (Field->getType()->isReferenceType()) { - Dst.setChild(*Field, SrcFieldLoc); - } else { - if (Value *Val = Env.getValue(*SrcFieldLoc)) - Env.setValue(*DstFieldLoc, *Val); - else - Env.clearValue(*DstFieldLoc); - } - } - - for (const auto &[Name, SynthFieldLoc] : Src.synthetic_fields()) { - if (SynthFieldLoc->getType()->isRecordType()) { - copyRecord(*cast<RecordStorageLocation>(SynthFieldLoc), - cast<RecordStorageLocation>(Dst.getSyntheticField(Name)), Env); - } else { - if (Value *Val = Env.getValue(*SynthFieldLoc)) - Env.setValue(Dst.getSyntheticField(Name), *Val); - else - Env.clearValue(Dst.getSyntheticField(Name)); - } + if (SrcType == DstType || (SrcDecl != nullptr && DstDecl != nullptr && + SrcDecl->isDerivedFrom(DstDecl))) { + for (auto [Field, DstFieldLoc] : Dst.children()) + copyField(Field, Src.getChild(*Field), DstFieldLoc, Dst, Env); + for (const auto &[Name, DstFieldLoc] : Dst.synthetic_fields()) + copySyntheticField(DstFieldLoc->getType(), Src.getSyntheticField(Name), + *DstFieldLoc, Env); + } else { + for (auto [Field, SrcFieldLoc] : Src.children()) + copyField(Field, SrcFieldLoc, Dst.getChild(*Field), Dst, Env); + for (const auto &[Name, SrcFieldLoc] : Src.synthetic_fields()) + copySyntheticField(SrcFieldLoc->getType(), *SrcFieldLoc, + Dst.getSyntheticField(Name), Env); } RecordValue *DstVal = &Env.create<RecordValue>(Dst); Env.setValue(Dst, *DstVal); } -bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1, - const Environment &Env1, - const RecordStorageLocation &Loc2, - const Environment &Env2) { +bool recordsEqual(const RecordStorageLocation &Loc1, const Environment &Env1, + const RecordStorageLocation &Loc2, const Environment &Env2) { LLVM_DEBUG({ if (Loc2.getType().getCanonicalType().getUnqualifiedType() != Loc1.getType().getCanonicalType().getUnqualifiedType()) { @@ -116,3 +132,5 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1, return true; } + +} // namespace clang::dataflow diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 04aa2831df0558..14060cbc5f3358 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -525,15 +525,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { if (LocSrc == nullptr || LocDst == nullptr) return; - // The assignment operators are different from the type of the destination - // in this model (i.e. in one of their base classes). This must be very - // rare and we just bail. - if (Method->getFunctionObjectParameterType() - .getCanonicalType() - .getUnqualifiedType() != - LocDst->getType().getCanonicalType().getUnqualifiedType()) - return; - copyRecord(*LocSrc, *LocDst, Env); // If the expr is a glvalue, we can reasonably assume the operator is diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp index cd6a37d370e854..dd109eb90984eb 100644 --- a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp @@ -198,7 +198,7 @@ TEST(RecordOpsTest, RecordsEqual) { }); } -TEST(TransferTest, CopyRecordFromDerivedToBase) { +TEST(TransferTest, CopyRecordBetweenDerivedAndBase) { std::string Code = R"( struct A { int i; @@ -212,8 +212,23 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) { // [[p]] } )"; + auto SyntheticFieldCallback = [](QualType Ty) -> llvm::StringMap<QualType> { + CXXRecordDecl *ADecl = nullptr; + if (Ty.getAsString() == "A") + ADecl = Ty->getAsCXXRecordDecl(); + else if (Ty.getAsString() == "B") + ADecl = Ty->getAsCXXRecordDecl() + ->bases_begin() + ->getType() + ->getAsCXXRecordDecl(); + else + return {}; + QualType IntTy = getFieldNamed(ADecl, "i")->getType(); + return {{"synth_int", IntTy}}; + }; + // Copy derived to base class. runDataflow( - Code, /*SyntheticFieldCallback=*/{}, + Code, SyntheticFieldCallback, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, ASTContext &ASTCtx) { Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); @@ -224,11 +239,38 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) { EXPECT_NE(Env.getValue(*A.getChild(*IDecl)), Env.getValue(*B.getChild(*IDecl))); + EXPECT_NE(Env.getValue(A.getSyntheticField("synth_int")), + Env.getValue(B.getSyntheticField("synth_int"))); copyRecord(B, A, Env); EXPECT_EQ(Env.getValue(*A.getChild(*IDecl)), Env.getValue(*B.getChild(*IDecl))); + EXPECT_EQ(Env.getValue(A.getSyntheticField("synth_int")), + Env.getValue(B.getSyntheticField("synth_int"))); + }); + // Copy base to derived class. + runDataflow( + Code, SyntheticFieldCallback, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); + + const ValueDecl *IDecl = findValueDecl(ASTCtx, "i"); + auto &A = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "a"); + auto &B = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "b"); + + EXPECT_NE(Env.getValue(*A.getChild(*IDecl)), + Env.getValue(*B.getChild(*IDecl))); + EXPECT_NE(Env.getValue(A.getSyntheticField("synth_int")), + Env.getValue(B.getSyntheticField("synth_int"))); + + copyRecord(A, B, Env); + + EXPECT_EQ(Env.getValue(*A.getChild(*IDecl)), + Env.getValue(*B.getChild(*IDecl))); + EXPECT_EQ(Env.getValue(A.getSyntheticField("synth_int")), + Env.getValue(B.getSyntheticField("synth_int"))); }); } diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index a8c282f140b4cd..0e9255737f6d64 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -2313,8 +2313,6 @@ TEST(TransferTest, AssignmentOperator_ArgByValue) { } TEST(TransferTest, AssignmentOperatorFromBase) { - // This is a crash repro. We don't model the copy this case, so no - // expectations on the copied field of the base class are checked. std::string Code = R"( struct Base { int base; @@ -2326,14 +2324,33 @@ TEST(TransferTest, AssignmentOperatorFromBase) { void target(Base B, Derived D) { D.base = 1; D.derived = 1; + // [[before]] D = B; - // [[p]] + // [[after]] } )"; runDataflow( Code, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, - ASTContext &ASTCtx) {}); + ASTContext &ASTCtx) { + const Environment &EnvBefore = + getEnvironmentAtAnnotation(Results, "before"); + const Environment &EnvAfter = + getEnvironmentAtAnnotation(Results, "after"); + + auto &BLoc = + getLocForDecl<RecordStorageLocation>(ASTCtx, EnvBefore, "B"); + auto &DLoc = + getLocForDecl<RecordStorageLocation>(ASTCtx, EnvBefore, "D"); + + EXPECT_NE(getFieldValue(&BLoc, "base", ASTCtx, EnvBefore), + getFieldValue(&DLoc, "base", ASTCtx, EnvBefore)); + EXPECT_EQ(getFieldValue(&BLoc, "base", ASTCtx, EnvAfter), + getFieldValue(&DLoc, "base", ASTCtx, EnvAfter)); + + EXPECT_EQ(getFieldValue(&DLoc, "derived", ASTCtx, EnvBefore), + getFieldValue(&DLoc, "derived", ASTCtx, EnvAfter)); + }); } TEST(TransferTest, AssignmentOperatorFromCallResult) { `````````` </details> https://github.com/llvm/llvm-project/pull/85064 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
