[analyzer] Teach CloneDetection about Qt Meta-Object Compiler

[xiangzhai]

Summary

Hi LLVM developers,

CloneChecker is very useful:

• Potential copy-paste error; did you really mean to use 'track' here? screenshot
• Duplicate code detected screenshot

It is able to detect Copy-paste issue acts as Coverity scan!

But too much False Positives! screenshot for Qt based project, for example, k3b so I simply add a filter for Qt Meta-Object Compiler moc_XXX.cpp please review my patch, give me some advice, thanks a lot!

Regards,
Leslie Zhai

[NoQ]

This checker would definitely benefit from suppression in all auto-generated files, which definitely includes MOC files.

However, there were also plans to tweak minimum clone sizes, which would also most likely suppress a lot of false positives.

Also, should we do it in CloneDetector (better performance) or in CloneChecker (cleaner framework), or maybe as a separate constraint on clone groups (hmm, sounds perfect in all senses)?

[xiangzhai]

Hi NoQ,

Sorry for my late reply! I have to look after my (almost 2 years old) little kid in the weekend :)

Also, should we do it in CloneDetector (better performance) or in CloneChecker (cleaner framework), or maybe as a separate constraint on clone groups (hmm, sounds perfect in all senses)?

I argue that it is better to do it in CloneDetector like my patch did, others' thoughts?

To Qt, there is not only auto-generation Meta Object Compiler, but also User Interface Compiler and D-Bus XML Compiler, please point out my fault if missed any, thanks! so we need a smarter Filter to get rid of moc_XXX, ui_XXX and dbus_XXX auto-generated source files.

But my monkey patch is very rough, just filter the moc_XXX fileName, for balancing performance and low false positive, I will update my patch next Monday, so welcome to discuss :)

Regards,
Leslie Zhai

[teemperor]

The constraints should be able to do the same filtering with the same performance in a cleaner way. So once D23418 is merged, we can put this code into a constraint and just prepend it to the list of constraints we use in the checker.

[xiangzhai]

Hi Raphael,

The father of CloneDetection :) thanks for your great job!

I will try your patch, is it rebase for the SVN trunk? almost for the nightly build, I want to detect Copy-paste issues for KDE projects, such as K3B, the real and big testcase for verifying whether or not False Positive! thanks again for your great job!

Regards,
Leslie Zhai

[xiangzhai]

Hi Raphael,

I updated my patch base on your job D23418 I put the code into a constraint AutoGeneratedCloneConstraint and just prepend it to the list of constraints used in the checker. and I will tweak it with a lot of real testcases: such as KDE projects. please review my patch, thanks a lot!

Regards,
Leslie Zhai

[xiangzhai]

Typo... it should be !D->isInvalidDecl().

[xiangzhai]

Fix my stupid copy-paste issue...

[NoQ]

lib/Analysis/CloneDetection.cpp

674

Hmm. Raphael: How often is D unavailable, or is invalid? I believe the checker currently only takes decls that come from checkASTCodeBody, in which case it's always safe; however there may be future plans to lift this contract.

681

Should we break here? There's no need to remove the same index twice.

lib/StaticAnalyzer/Checkers/CloneChecker.cpp

84 ↗

(On Diff #93453)

Would it be beneficial to apply this constraint first, to quickly filter out autogenerated code and save some work for other constraints?

98 ↗

(On Diff #93453)

Should we add the new constraint here as well?

[xiangzhai]

Hi Artem,

Thanks for your review! I have updated my patch as you suggest, please review it, thanks a lot!

Regards,
Leslie Zhai

[teemperor]

lib/Analysis/CloneDetection.cpp

666

You could just use CloneConstraint::filterGroups() instead of doing your own loop and erasing.

674

No such plans for invalid declarations. And if we would want to filter them, we should in the CloneDetector::analyzeCodeBody()

lib/StaticAnalyzer/Checkers/CloneChecker.cpp

97 ↗

(On Diff #94314)

This is indeed unnecessary as Artem said (we already removed it above from all clone groups).

unittests/Analysis/CloneDetectionTest.cpp

76 ↗

(On Diff #94314)

It's not relevant for the test, please remove.

94 ↗

(On Diff #94314)

It's not relevant for the test, please remove.

[NoQ]

lib/StaticAnalyzer/Checkers/CloneChecker.cpp

97 ↗

(On Diff #94314)

Aha, my bad.

[xiangzhai]

Hi Raphael,

Thanks for your review!

I have updated my patch as you suggested, and make check-clang-analysis for tripple check my patch, please review it, thanks a lot!

Regards,
Leslie Zhai

[NoQ]

Could you add a test, like a regular test in test/Analysis/copypaste/? Like, make a file with _moc in it, and demonstrate that the checker doesn't warn, despite having obvious clones otherwise.

lib/Analysis/CloneDetection.cpp

670

I guess you can use SourceManager::getFilename().

[xiangzhai]

Hi Artem,

Please review my updated patch, I use SourceManager's getFilename and add some testcases, thanks a lot!

Regards,
Leslie Zhai

[NoQ]

Thanks! Because LLVM's Illinois license is rather permissive than copyleft, we try to avoid stuff copied from GPL/LGPL software (such as Qt or K3B) in our codebase. The code doesn't seem to have been copy-pasted from a copyleft project, but I guess it's better to rename the file to avoid referencing to K3B, and i added inline comments regarding moc intro comments.

lib/Analysis/CloneDetection.cpp

669

Maybe .startswith("moc_") would be more accurate?

In fact, you could also try to use a single LLVM regex here.

test/Analysis/copypaste/k3blib_automoc.cpp

5–7 ↗

(On Diff #97047)

Other LLVM contributors are free to edit this file, and i doubt it was autogenerated; i believe these comments should be removed.

That said, there should be a comment explaining why the file has no warning. Eg.:

// Because files that have `_automoc' in their names are most likely autogenerated,
// we suppress copy-paste warnings here.

// expected-no-diagnostics

(same in the other file)

16 ↗

(On Diff #97047)

My idea regarding the comment above would make this comment unnecessary. (same in the other file)

test/Analysis/copypaste/moc_k3bactivepipe.cpp

5–13 ↗

(On Diff #97047)

This file wasn't in fact created by moc, does not seem to be anyhow related to k3b; i believe these comments should be removed.

[xiangzhai]

Hi Artem,

Thanks for your review!

Because LLVM's Illinois license is rather permissive than copyleft, we try to avoid stuff copied from GPL/LGPL software (such as Qt or K3B) in our codebase. The code doesn't seem to have been copy-pasted from a copyleft project, but I guess it's better to rename the file to avoid referencing to K3B.

Sorry for my mistake! I have rename the testcase to moc_autogenerated.cpp and autogenerated_automoc.cpp. I am the maintainer of K3B, if you find any bug when burning ISO please report bug to me :)

Other LLVM contributors are free to edit this file, and i doubt it was autogenerated; i believe these comments should be removed.

Sorry for my fault!

/* This file is autogenerated, do not edit*/

it is autogenerated by MOC, I removed it!

And I use a single line LLVM regex instead of std::string functions, please review my patch, thanks a lot!

Regards,
Leslie Zhai

[v.g.vassilev]

In general, I think we should support such use-cases. It seems cleaner to me if we provide some sort of callbacks where the users can specify their custom false positive filters. I've discussed this in brief with Raphael and we were planning to write this up.

[xiangzhai]

Hi Vassil,

The first one considered about CopyPaste detection Thanks for your reply! Let's do it together and Happy International Labor Day :)

Regards,
Leslie Zhai

[zaks.anna]

Should this revision be "abandoned" or is the plan to iterate on it?

[xiangzhai]

Dear Anna,

Long time no see, miss you :)

Should this revision be "abandoned" or is the plan to iterate on it?

I wanna the plan to iterate on it, Artem reviewed the patch, and I fixed the issue as he suggested, ran make check-clang-analysis also used it for checking real project K3b's Copy-paste issue, worked!

Hi Vassil,

... Thanks for your reply! Let's do it together and Happy International Labor Day :)

But Vassil and Raphael dis not reply me.

Could you kindly review it, if LGTU, may I commit it? thanks a lot!

Regards,
Leslie Zhai

[teemperor]

Sorry for the delay, we got stuck because hard coding a certain file pattern into the clang source code doesn't seem to be a optimal solution (e.g. every user that has a different set of generated files needs to patch clang to hide his specific reports). I would prefer if we could get this into a variable that the user can change dynamically.

Would it solve your use case if allow specifying file patterns via the command line like this -analyzer-config alpha.clone.CloneChecker:IgnoredFiles=moc_*;*.pb.h;*.pb.cc? If yes, please update this PR accordingly and then I think this patch is good to go.

Thanks for the work!

[v.g.vassilev]

I agree with Raphael. If you need to have more fine grained control over the translation unit (TU) I think we can have suppressing false positives via comments (@NoQ would know more).

Eg.

// File.h defining moc_* yields a lot of false positives

// alpha.clone.CloneChecker:Ignored: some_regex
...

This would affect only the TU which include File.h.

[xiangzhai]

Dear Raphael,

Thanks for your suggestion!

Would it solve your use case if allow specifying file patterns via the command line like this -analyzer-config

Fixed!

$ /home/zhaixiang/project/llvm/build/./bin/clang -cc1 -internal-isystem /home/zhaixiang/project/llvm/build/lib64/clang/5.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -std=c++11 -analyzer-checker=alpha.clone.CloneChecker -analyzer-config alpha.clone.CloneChecker:IgnoredFilesPattern="moc_|ui_|dbus_|.*_automoc" -verify /home/zhaixiang/project/llvm/tools/clang/test/Analysis/copypaste/not-autogenerated.cpp

I ran make check-clang-analysis and tested it for checking the real K3B project's Copy-paste issue to filter Meta Object Compiler, User Interface Compiler, D-Bus XML Compiler, worked!

And it is able to specify the IgnoredFilesPattern to filter auto-generated files for different frameworks, please review my patch, if LGTU, may I commit it? thanks a lot!

Regards,
Leslie Zhai

[teemperor]

See my inline comments about technical changes, but otherwise this looks ready to land. Please update and I'll give green light ASAP. Thanks!

lib/Analysis/CloneDetection.cpp

667

You can skip the const Decl *D = S.getContainingDecl(); and just do const SourceManager &SM = S.getASTContext().getSourceManager();

670

Let's get the basename with the LLVM path class instead:
http://llvm.org/docs/doxygen/html/namespacellvm_1_1sys_1_1path.html#a799b002e67dcf41330fa8d6fa11823dc

E.g. moc_test\.cpp is a valid filename on Linux and then this code isn't doing the right thing.

675

Hmm, we can't filter by file extension this way? Can we remove this extension stripping and just make people type the file extension in the filter, this feels more intuitive.

676

Artem suggested we could move this Regex out of the loop, I think we should even make it a member of the AutoGeneratedCloneConstraint so that we only parse/generate the regex state machine once per invocation. Currently we reparse this Regex a few thousand times and performance is quite important in this Checker.

[xiangzhai]

Dear Raphael,

I updated my patch as you suggested, please review it, thanks a lot!

And I noticed that you are doing Performance optimizations for the CloneChecker D34182 I will rebase my patch if you kindly commit.

Regards,
Leslie Zhai

[teemperor]

Please check the last inline comment and then feel free to commit it with the suggested fix. And I wanted to wait for review on the other performance patches, so you can push this now.

Thanks for the work!

lib/Analysis/CloneDetection.cpp

666

Sorry, I what I wanted to suggest is: make this a member variable of the AutoGeneratedCloneConstraint class. Moving this out of the loop actually doesn't change anything for the checker (because the first constraint get's a list of single-sequence groups, so we still call this function N times for N functions). So something like llvm::Regex AutoGeneratedCloneConstraint::IgnoredFilesRegex :)

[xiangzhai]

Dear Raphael,

Sorry I am not available, I am looking after my little kid, see you next week, rebase perhaps.

Thanks,
Leslie Zhai

[xiangzhai]

Dear Raphael,

I updated my patch as you suggested, may I commit it? thanks!

Regards,
Leslie Zhai

[teemperor]

LGTM, thanks for the patch!

[v.g.vassilev]

cfe/trunk/include/clang/Analysis/CloneDetection.h

324 ↗

(On Diff #102984)

Shouldn't the name be more generic. What this essentially does is to filter out false positives according to a regex...

[xiangzhai]

cfe/trunk/include/clang/Analysis/CloneDetection.h

324 ↗

(On Diff #102984)

At the very beginning, it is by regex match the filename, it is still very rough! but in future it is able to filter by looking for QT_BEGIN_MOC_NAMESPACE macro or qt_meta_ prefix function in the ASTContext? and there might be other auto-generated mechanism for different framework, such as Gtk and sort of open source GUI libraries. so perhaps AutoGeneratedCloneConstraint is better name, I am not good at naming, it is difficult to name my little kid :)

[teemperor]

cfe/trunk/include/clang/Analysis/CloneDetection.h

324 ↗

(On Diff #102984)

Yeah, maybe we should have named this FilenamePatternConstraint or something like that...

include/clang/Analysis/CloneDetection.h

 
 namespace clang {
 
 class Stmt;
 class Decl;
 class VarDecl;
 class ASTContext;
 class CompoundStmt;
+class FileEntry;
 
 /// \brief Identifies a list of statements.
 ///
 /// Can either identify a single arbitrary Stmt object, a continuous sequence of
 /// child statements inside a CompoundStmt or no statements at all.
 class StmtSequence {
   /// If this object identifies a sequence of statements inside a CompoundStmt,
   /// S points to this CompoundStmt. If this object only identifies a single
public:
   /// \brief Searches the provided statements for pairs of clones that don't
   ///        follow the same pattern when referencing variables.
   /// \param Result Output parameter that will contain the clone pairs.
   /// \param MinGroupComplexity Only clone pairs in which the clones have at
   ///                           least this complexity value.
   void findSuspiciousClones(std::vector<SuspiciousClonePair> &Result,
                             unsigned MinGroupComplexity);
 
+  /// \brief Get the FileEntry for Decl.
+  /// \param Decl.
+  /// \param SourceManager.
+  /// \return FileEntry.
+  const clang::FileEntry *getFileEntryForDecl(const clang::Decl *decl, 
+                                              clang::SourceManager *sourceManager);
+
 private:
   /// Stores all encountered StmtSequences alongside their CloneSignature.
   std::vector<std::pair<CloneSignature, StmtSequence>> Sequences;
 };
 
 } // end namespace clang
 
 #endif // LLVM_CLANG_AST_CLONEDETECTION_H

lib/Analysis/CloneDetection.cpp

public:
   explicit CloneSignatureGenerator(CloneDetector &CD, ASTContext &Context)
       : CD(CD), Context(Context) {}
 
   /// \brief Generates signatures for all statements in the given function body.
   void consumeCodeBody(const Stmt *S) { generateSignatures(S, ""); }
 };
 } // end anonymous namespace
 
+const clang::FileEntry *CloneDetector::getFileEntryForDecl(const clang::Decl *decl, 
+                                                           clang::SourceManager * sourceManager) {
+    if (!decl || !sourceManager)
+      return nullptr;
+    clang::SourceLocation sLoc = decl->getLocation();
+    clang::FileID fileID = sourceManager->getFileID(sLoc);
+    return sourceManager->getFileEntryForID(fileID);
+}
+
 void CloneDetector::analyzeCodeBody(const Decl *D) {
   assert(D);
+  assert(D->getASTContext());
+  const clang::FileEntry *fileEntry = getFileEntryForDecl(D, 
+                                        &D->getASTContext().getSourceManager());
+  // Filter Qt's Meta-Object Compiler (moc) http://doc.qt.io/qt-5/moc.html
+  // The false positive is too high for moc_XXX.cpp during scan-build
+  if (fileEntry && std::string(fileEntry->getName()).find("moc_") == 0)
+    return;
   assert(D->hasBody());
   CloneSignatureGenerator Generator(*this, D->getASTContext());
   Generator.consumeCodeBody(D->getBody());
 }
 
 void CloneDetector::add(const StmtSequence &S,
                         const CloneSignature &Signature) {
   Sequences.push_back(std::make_pair(Signature, S));
bool containsGroup(CloneDetector::CloneGroup &Group,
   }
   return true;
 }
 } // end anonymous namespace
 
 namespace {
 /// \brief Wrapper around FoldingSetNodeID that it can be used as the template
 ///        argument of the StmtDataCollector.
 class FoldingSetNodeIDWrapper {
 
   llvm::FoldingSetNodeID &FS;
 
 public:
   FoldingSetNodeIDWrapper(llvm::FoldingSetNodeID &FS) : FS(FS) {}
 
   void update(StringRef Str) { FS.AddString(Str); }
 };
 } // end anonymous namespace
 
 /// \brief Writes the relevant data from all statements and child statements
 ///        in the given StmtSequence into the given FoldingSetNodeID.
 static void CollectStmtSequenceData(const StmtSequence &Sequence,
                                     FoldingSetNodeIDWrapper &OutputData) {
   for (const Stmt *S : Sequence) {
     StmtDataCollector<FoldingSetNodeIDWrapper>(S, Sequence.getASTContext(),
                                                OutputData);
 
     for (const Stmt *Child : S->children()) {
       if (!Child)
         continue;
 
       CollectStmtSequenceData(StmtSequence(Child, Sequence.getASTContext()),