improved structural equivalence checking

[balazske]

Added check for CXXMethodDecl (not complete: some template cases missing?). Cleaned up check of FunctionTemplateDecl. Added unit tests.

Diagnostic messages not added.

[balazske]

The produced diagnostic messages depend on the order in source code of testing structural equivalence of elements. If a test for an element (for example field) fails the check is stopped and no other elements (for example methods) are checked. Probably it is better to check for everything and produce a message for every problem.

The check for fields in RecordDecl was moved to before check for methods, otherwise the check may fail at a method first and produce different message than before, this causes one unit test to fail.

[balazske]

Code was updated.

[martong]

Balazs, I think we should split this patch into two:

1) All those improvements which are connected to the equivalence check of free functions and member functions (noexcept, && qualifier, const qualifier, etc). This includes the new function

+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+                             CXXMethodDecl *Method1, CXXMethodDecl *Method2) {

and its tests too.

2) Checking the member functions as well in a RecordDecl.

I think 1) is an immediate improvement and @Xazax-hun would accept that as well. Just think about out-of-class definition of member functions ... it is definitely an improvement that we can distinguish a const qualified member function from a non-const one.

2 might cause some degradation since our general structural eq checking is still buggy (and also we create sometimes a faulty AST with respect to classtemplates and specializations). Thus, perhaps 2) should be merged after we are done with #384 / 1)

[Xazax-hun]

I support martong's proposal!

[balazske]

Code updated: Now only the check for CXXMethodDecl is implemented. The "duplicate FunctionTemplateDecl" problem in Finish will be corrected in a separate PR.

[balazske]

Maybe final should be checked too?

[martong]

With respect to functions, if we check purity then it seems logical to check finality too.

[balazske]

There is a class named ODRHash, is it not useful for these checks? Edit: It is not directly useful: We want to check these "skipped" functions too.

 void ODRHash::AddFunctionDecl(const FunctionDecl *Function) {
   assert(Function && "Expecting non-null pointer.");

   // Skip hashing these kinds of function.
   if (Function->isImplicit()) return;
   if (Function->isDefaulted()) return;
   if (Function->isDeleted()) return;
   if (!Function->hasBody()) return;
   if (!Function->getBody()) return;

[martong]

@balazske Yes, I think we try to implement something similar here. So if ODRHash is stable and full we could use that instead of the whole messy ASTStructuralEquivalence. Maybe in the future we can/should change to that.

[balazske]

Code was updated, the new warnings about incompatible methods are still not implemented.

[balazske]

Is it possible to merge this into ctu-clang6 on local computer and push it to ericsson repo (without pull request)? (I do not want to use "web editor" and there exists the branch on local repository that would be easy to merge. But this would make 2 commits (the existing and the merge) and probably other not wanted things. Otherwise the web editor is usable but no compile is done after it to check for basic syntax errors.)

[martong]

@balazske Why do you want to push it to ericsson repo? Do you want it to be in a separate branch in the ericsson repo? You can test your changes even if they are kept in your remote repo (github/balazske/clang).

Here is how you can do that. You can merge the tip of Ericsson/ctu-clang6 branch into you local branch. Then push that to a branch in your remote repo. Then you can modify the jenkins job http://cc.elte.hu:15009/job/test-clang-with-projects-egbomrt/ to use your repo and branch.

[martong]

Started a dry run in CI: http://cc.elte.hu:15009/job/test-clang-with-projects-egbomrt/9/

Expect the results in 9 hours.

[Xazax-hun]

In bitcoin the failed actions increased from 20 to 45. We should check if we caught more bugs with the checks or there are bugs in the checks themselves.

[martong]

Yes. Balazs, could you please check why we get so many new assertions:

Assertion `isa(Val) && "cast() argument of incompatible type!"' failed. [33]

http://cc.elte.hu:15009/job/test-clang-with-projects-egbomrt/9/HTML_Report/

[balazske]

How to find the log files (or other information) with this assertion? (Is there filename and line number, and call stack?)

[martong]

I think the best is to reproduce locally, on you machine the assert. You should use the latest CodeChecker configured to use your built clang and then a ctu-analyze.

cd bitcoin
./configure
CodeChecker analyze --analyzers clangsa --ctu-collect -o ./reports ./compile_commands.json -j4
CodeChecker analyze --analyzers clangsa --ctu-analyze -o ./reports ./compile_commands.json -j4

Use these setting to configure bitcoin:

    {
      "name": "Bitcoin",
      "url": "https://github.com/bitcoin/bitcoin.git",
      "tag": "v0.16.0",
      "configure_command": "./autogen.sh && ./configure --disable-wallet --disable-static --disable-tests"
    },

[martong]

Note, bitcoin CTU run is fast (20 minutes)

[balazske]

Or 200?

[martong]

Hmm, should not be that slow. Are you using a debug build? I generally use a release_assert bulid, then if needed I debug the individual analyzer commands with a debug build.

[martong]

@balazske Does your last commit fix the extra assertions?

[martong]

Just started a new build: http://cc.elte.hu:15009/job/test-clang-with-projects-egbomrt/10

[balazske]

It fixes the cast error. There is an ToDecl->hasBody() assertion that is not fixed. But the fix for that is easy (setEncounteredUnsupportedNode is not called), not committed yet. (The CXXStdInitializerListExpr is not handled at import.)

[balazske]

With the second (not included yet) fix Bitcoin can be analyzed without failures. Should implement import of CXXStdInitializerListExpr (it looks easy to do)? (If yes it belongs to a different Pull Request.)

[martong]

Yes, please do a different pull request for the initializer list.

[balazske]

Import of std initializer list: #391 .

[balazske]

LLVM review: https://reviews.llvm.org/D48628

[balazske]

Backport from LLVM: #425