Ping

• Fixes from review comments, added C++ version test.

Ping

• Re-design of eval functions.
• Added a C++ test with fopen-looking function.

• Various code cleanups. Eval functions use CallEvent, CallExpr is removed from state.

• Rename to DefaultOperatorNewAlignmentCheck.

Using CallDescriptionMap.

I try to improve StreamChecker instead.

Fixed the test, fixed problems in list.rst.

Rebase and update according to comments.
C++17 related changes not implemented yet (possible check for the called allocation function).

C++17 makes things more difficult because the align is probably handled by operator new, probably not, depending on the defined allocation functions. This can be observed only with a non clang-tidy checker (we could compute the used alignment?). Probably the CERT rule is from the time before C++17. It looks like that by default the alignment is handled correctly in C++17 so the whole check is out of scope for that language version.

Code of StreamChecker does not look much better, it uses deprecated eval::Call and does not check for escape. But it handles more functions and error at open (still not freopen). A mixture of both would be a better option.

OK
Probably the ClassTemplateSpec can not be handled in liberal way because the AST data structure for template specializations do not allow multiple instances with same argument values?

Rename of constants:
https://reviews.llvm.org/D67174

• Added comment about repeated enums in C99.

In D64480#1653629, @shafik wrote:

It is worth noting that:

typedef int T;
typedef int T;

is not valid C99 see godbolt

• Removed unneeded structs.

Improved the tests.

• Renamed ImportOverrides, additional check in test.

• Simplified field matcher in tests.

• Small update to newer C++ syntax, use std::pair in test.

I remember that we had "problems" with nonsense ODR warnings where both declarations are the same. Probably the reason for it was this cache problem. Still the NonEquivalentDecls as cache is useful to filter out the non-equivalences that were already encountered to not produce repeated warnings for them.

In D66538#1642883, @martong wrote:

There is a third test which could be useful to test whether there is no faulty cache entries there:

+TEST_F(StructuralEquivalenceCacheTest, DISABLED_NonEq) {
...
+}

This is somewhat the same check that is done in the current tests when the NonEquivalentDecls is checked to not contain any pairs of equivalent Decls. (Specially this line:

EXPECT_FALSE(isInNonEqCache(
      findDeclPair<FunctionDecl>(TU, functionDecl(hasName("x")))));

)

• Updated tests.

The structural equivalency check works (with this patch) the following way: To check a (A1,A2) pair we collect further (X1,X2) pairs that should be checked and put these in DeclsToCheck (if not already there). The check succeeds if every pair is equivalent. The pairs (A1,X2) and (A1,Y2) are checked independently. A true return value from any of the IsStructurallyEquivalent functions means that we did not found non-equivalence in the internal node data but the pairs in DeclsToCheck should be checked too. Return value of false means a non-equivalence in the internal data only is found with the (X1,X2) pair that is currently checked, therefore we found non-equivalence for the original pair and for (X1,X2) too. Again, with the new code the test CheckCommonEquivalence(D1, D2) && CheckKindSpecificEquivalence(D1, D2) fails only if we found difference in the internal node data and we can be sure that D1 and D2 are non-equivalent. The old code does not have this property.

In D66538#1641310, @martong wrote:

It looks like that the original code is correct in the decision of structural equivalence of the original pair. If we have an (A,B) and (A,C) to compare, B and C are in different decl chain, then (A,B) or (A,C) will be non-equivalent (unless B and C are equivalent, but what to do in this case?). The problem was that the code assumed that in this case always A and B (or A and C) are non-equivalent. If NonEquivalentDecls is not filled in this case (or not used at all) the problem does not exist.

I think we must not update the NonEquivalentDecls cache at that level, because as we've seen (during the discussion of https://reviews.llvm.org/D62131) it may store the wrong pairs.
However, it is still okay to cache the inequivalent decls in one level upper, right after the Finish returns.
See this diff https://github.com/martong/clang/compare/NonEqDecls_cache_in_an_upper_level~...martong:NonEqDecls_cache_in_an_upper_level?expand=1
Right now I started a build on our CI to see if it causes any slowdown.

This change is obsolete in the current code. Casting related changes do not look important (and should be a new patch anyway).

A "quick fix" for the problem:
https://reviews.llvm.org/D66538

Example about how to get wrong things into NonEquivalentDecls:
We want to compare class C for structural equivalence in the following codes:

class A {}; class B {int i;}; void x(A *); void y(A *); class C { friend void x(A *); friend void y(A *); };

and

class A {}; class B {int i;}; void x(A *); void y(B *); class C { friend void x(A *); friend void y(B *); };

The result is false for C but the NonEquivalentDecls will contain after the compare a void x(A*)<->void x(A*) pair.

LLDB probably does the import in other way. CTU test fails without the fix:

FAIL: Clang :: Analysis/ctu-main.cpp (540 of 15341)
******************** TEST 'Clang :: Analysis/ctu-main.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   rm -rf build/Release/tools/clang/test/Analysis/Output/ctu-main.cpp.tmp && mkdir build/Release/tools/clang/test/Analysis/Output/ctu-main.cpp.tmp
: 'RUN: at line 2';   mkdir -p build/Release/tools/clang/test/Analysis/Output/ctu-main.cpp.tmp/ctudir
: 'RUN: at line 3';   build/Release/bin/clang -cc1 -internal-isystem build/Release/lib/clang/10.0.0/include -nostdsysteminc -triple x86_64-pc-linux-gnu    -emit-pch -o build/Release/tools/clang/test/Analysis/Output/ctu-main.cpp.tmp/ctudir/ctu-other.cpp.ast llvm-project/clang/test/Analysis/Inputs/ctu-other.cpp
: 'RUN: at line 5';   build/Release/bin/clang -cc1 -internal-isystem build/Release/lib/clang/10.0.0/include -nostdsysteminc -triple x86_64-pc-linux-gnu    -emit-pch -o build/Release/tools/clang/test/Analysis/Output/ctu-main.cpp.tmp/ctudir/ctu-chain.cpp.ast llvm-project/clang/test/Analysis/Inputs/ctu-chain.cpp
: 'RUN: at line 7';   cp llvm-project/clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt build/Release/tools/clang/test/Analysis/Output/ctu-main.cpp.tmp/ctudir/externalDefMap.txt
: 'RUN: at line 8';   build/Release/bin/clang -cc1 -internal-isystem build/Release/lib/clang/10.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -triple x86_64-pc-linux-gnu    -analyzer-checker=core,debug.ExprInspection    -analyzer-config experimental-enable-naive-ctu-analysis=true    -analyzer-config ctu-dir=build/Release/tools/clang/test/Analysis/Output/ctu-main.cpp.tmp/ctudir    -verify llvm-project/clang/test/Analysis/ctu-main.cpp
: 'RUN: at line 13';   build/Release/bin/clang -cc1 -internal-isystem build/Release/lib/clang/10.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -triple x86_64-pc-linux-gnu    -analyzer-checker=core,debug.ExprInspection    -analyzer-config experimental-enable-naive-ctu-analysis=true    -analyzer-config ctu-dir=build/Release/tools/clang/test/Analysis/Output/ctu-main.cpp.tmp/ctudir    -analyzer-config display-ctu-progress=true 2>&1 llvm-project/clang/test/Analysis/ctu-main.cpp | build/Release/bin/FileCheck llvm-project/clang/test/Analysis/ctu-main.cpp
--
Exit Code: 134

• Moved 'ctor initializer' import before import of body.

• Moved SVEBuiltins test instantiation to non-different position.

• Using StringRef, added comment.

In this case the problem was that some of the flags of the already created and inserted ToFunction are not initialized. During the import of some "ctor initializers" this non-complete ToFunction may be accessed somehow (by structural equivalence or other code) and an assert comes because invalid value. (Maybe in the test code the second constructor is imported as "ctor initializer" during the import of the first.)

• [CrossTU] Fix problem with CrossTU AST load limit and progress messages.
• Reformat.

Rename of variables and reformat.

I discovered that there are problems with this change. The number of loaded AST units is not incremented correctly and the "CTU loaded AST file" is displayed for every access to a AST unit (even if it was got from cache). The problem is that the counting and print of AST loads does not handle the case when the load returned a cached result (and no real loading happened). This causes "annoying" messages and the AST unit limit is not handled correctly.