martong created this revision. martong added reviewers: NoQ, steakhal. Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun. Herald added a reviewer: Szelethus. Herald added a project: All. martong requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits.
Fixes https://github.com/llvm/llvm-project/issues/55546 The assertion mentioned in the issue is triggered because an inconsistency is formed in the Sym->Class and Class->Sym relations. A simpler but similar inconsistency is demonstrated here: https://reviews.llvm.org/D114887 . Previously in `removeMember`, we didn't remove the old symbol's Sym->Class relation. Back then, we explained it with the following two bullet points: > 1. This way constraints for the old symbol can still be found via it's > > equivalence class that it used to be the member of. > > 2. Performance and resource reasons. We can spare one removal and thus one > > additional tree in the forest of `ClassMap`. This patch do remove the old symbol's Sym->Class relation in order to keep the Sym->Class relation consistent with the Class->Sym relations. Point 2) above has negligible performance impact, empirical measurements do not show any noticeable difference in the run-time. Point 1) above seems to be a not well justified statement. This is because we cannot create a new symbol that would be equal to the old symbol after the simplification had happened. The reason for this is that the SValBuilder uses the available constant constraints for each sub-symbol. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D126281 Files: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp clang/test/Analysis/symbol-simplification-assertion.c Index: clang/test/Analysis/symbol-simplification-assertion.c =================================================================== --- /dev/null +++ clang/test/Analysis/symbol-simplification-assertion.c @@ -0,0 +1,28 @@ +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=true \ +// RUN: -verify + +// Here we test that no assertion is fired during symbol simplification. +// Related issue: https://github.com/llvm/llvm-project/issues/55546 + +// expected-no-diagnostics + +extern void abort() __attribute__((__noreturn__)); +#define assert(expr) ((expr) ? (void)(0) : abort()) + +void clang_analyzer_printState(); +void clang_analyzer_dump(long); +void clang_analyzer_eval(int); + +int a, b, c; +long L, L1; +void test() { + assert(a + L + 1 + b != c); + assert(L == a); + assert(c == 0); + L1 = 0; + assert(a + L1 + 1 + b != c); + assert(a == 0); // no-assertion +} Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp +++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp @@ -2508,12 +2508,6 @@ SymbolSet ClsMembers = getClassMembers(State); assert(ClsMembers.contains(Old)); - // We don't remove `Old`'s Sym->Class relation for two reasons: - // 1) This way constraints for the old symbol can still be found via it's - // equivalence class that it used to be the member of. - // 2) Performance and resource reasons. We can spare one removal and thus one - // additional tree in the forest of `ClassMap`. - // Remove `Old`'s Class->Sym relation. SymbolSet::Factory &F = getMembersFactory(State); ClassMembersTy::Factory &EMFactory = State->get_context<ClassMembers>(); @@ -2527,6 +2521,12 @@ ClassMembersMap = EMFactory.add(ClassMembersMap, *this, ClsMembers); State = State->set<ClassMembers>(ClassMembersMap); + // Remove `Old`'s Sym->Class relation. + ClassMapTy Classes = State->get<ClassMap>(); + ClassMapTy::Factory &CMF = State->get_context<ClassMap>(); + Classes = CMF.remove(Classes, Old); + State = State->set<ClassMap>(Classes); + return State; }
Index: clang/test/Analysis/symbol-simplification-assertion.c =================================================================== --- /dev/null +++ clang/test/Analysis/symbol-simplification-assertion.c @@ -0,0 +1,28 @@ +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=true \ +// RUN: -verify + +// Here we test that no assertion is fired during symbol simplification. +// Related issue: https://github.com/llvm/llvm-project/issues/55546 + +// expected-no-diagnostics + +extern void abort() __attribute__((__noreturn__)); +#define assert(expr) ((expr) ? (void)(0) : abort()) + +void clang_analyzer_printState(); +void clang_analyzer_dump(long); +void clang_analyzer_eval(int); + +int a, b, c; +long L, L1; +void test() { + assert(a + L + 1 + b != c); + assert(L == a); + assert(c == 0); + L1 = 0; + assert(a + L1 + 1 + b != c); + assert(a == 0); // no-assertion +} Index: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp +++ clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp @@ -2508,12 +2508,6 @@ SymbolSet ClsMembers = getClassMembers(State); assert(ClsMembers.contains(Old)); - // We don't remove `Old`'s Sym->Class relation for two reasons: - // 1) This way constraints for the old symbol can still be found via it's - // equivalence class that it used to be the member of. - // 2) Performance and resource reasons. We can spare one removal and thus one - // additional tree in the forest of `ClassMap`. - // Remove `Old`'s Class->Sym relation. SymbolSet::Factory &F = getMembersFactory(State); ClassMembersTy::Factory &EMFactory = State->get_context<ClassMembers>(); @@ -2527,6 +2521,12 @@ ClassMembersMap = EMFactory.add(ClassMembersMap, *this, ClsMembers); State = State->set<ClassMembers>(ClassMembersMap); + // Remove `Old`'s Sym->Class relation. + ClassMapTy Classes = State->get<ClassMap>(); + ClassMapTy::Factory &CMF = State->get_context<ClassMap>(); + Classes = CMF.remove(Classes, Old); + State = State->set<ClassMap>(Classes); + return State; }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
