[analyzer] Prevent misuses of -analyze-function

[steakhal]

Summary

Sometimes when I pass the mentioned option I forget about passing the parameter list for c++ sources.
It would be also useful newcomers to learn about this.

This patch introduces some logic checking common misuses involving -analyze-function.

[martong]

Very good! If we had this earlier it could have saved me some frustrating debugging.

clang/test/Analysis/analyze-function-guide.cpp

18

So we need -Dbool=_Bool trickery to be able to parse the function in C, okay. I think it would have been simpler to have fizzbuzz(int x, int y) instead.

49

We should have a matching case with -x c as well.

[grandinj]

Nice quality-of-life improvement!

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

506

Every top-level function was skipped

[steakhal]

Thanks for the reviews

clang/test/Analysis/analyze-function-guide.cpp

18

I'm intentionally demonstrating this.
Since even in C++ mode, we need to match against _Bool.
Hopefully, this will help fellow developers for this detail.

[steakhal]

Landed with the requested changes.

[NoQ]

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

523

🥺

[NoQ]

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

523

You can use this for a test, should be in an .m file:

@interface MyClass
-(int) messageWithFoo: (int)foo bar: (int)bar;
@end

@implementation MyClass
-(int) messageWithFoo: (int)foo bar: (int)bar {
  return foo + bar;
}
@end

(reacts to -analyze-function="-[MyClass messageWithFoo:bar:]")

[NoQ]

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

523

Oh wait, this is already committed. I guess I can do that myself.

[steakhal]

It broke some bots. I had to revert it in minutes it landed. Yet to be investigated how did it break something.
Mind that I dont have any experience with objC.

[steakhal]

• pin target triple to (possibly) resolve the build bot failure
• add obj-c test case

[NoQ]

Thanks!!

[steakhal]

Ah, it seems like pinning the target triple was not enough.
I'm stuck at this point.

[NoQ]

Can you post a link to a specific buildbot failure so that we could look at it together?

[steakhal]

In D118690#3305086, @NoQ wrote:

Can you post a link to a specific buildbot failure so that we could look at it together?

Sure!

I received these three mails:
https://lab.llvm.org/buildbot#builders/67/builds/5824
https://lab.llvm.org/buildbot#builders/91/builds/3404
https://lab.llvm.org/buildbot#builders/98/builds/12849

[steakhal]

I've decided to remove the offending RUN lines.
It's less than ideal, but TBH I think it doesn't really matter.

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

 
 static bool fileContainsString(StringRef Substring, ASTContext &C) {
   const SourceManager &SM = C.getSourceManager();
   FileID FID = SM.getMainFileID();
   StringRef Buffer = SM.getBufferOrFake(FID).getBuffer();
   return Buffer.contains(Substring);
 }
 
+static void reportAnalyzerFunctionMisuse(const AnalyzerOptions &Opts,
+                                         const ASTContext &Ctx) {
+  llvm::errs() << "Every top-level functions were skipped.\n";
+
+  if (!Opts.AnalyzerDisplayProgress)
+    llvm::errs() << "Pass the -analyzer-display-progress for tracking which "
+                    "functions are analyzed.\n";
+
+  bool HasBrackets =
+      Opts.AnalyzeSpecificFunction.find("(") != std::string::npos;
+
+  if (Ctx.getLangOpts().CPlusPlus && !HasBrackets) {
+    llvm::errs()
+        << "For analyzing C++ code you need to pass the function parameter "
+           "list: -analyze-function=\"foobar(int, _Bool)\"\n";
+  } else if (!Ctx.getLangOpts().CPlusPlus && HasBrackets) {
+    llvm::errs() << "For analyzing C code you shouldn't pass the function "
+                    "parameter list, only the name of the function: "
+                    "-analyze-function=foobar\n";
+  }
+}
+
 void AnalysisConsumer::runAnalysisOnTranslationUnit(ASTContext &C) {
   BugReporter BR(*Mgr);
   TranslationUnitDecl *TU = C.getTranslationUnitDecl();
   if (SyntaxCheckTimer)
     SyntaxCheckTimer->startTimer();
   checkerMgr->runCheckersOnASTDecl(TU, *Mgr, BR);
   if (SyntaxCheckTimer)
     SyntaxCheckTimer->stopTimer();
void AnalysisConsumer::runAnalysisOnTranslationUnit(ASTContext &C) {
   if (Mgr->shouldInlineCall())
     HandleDeclsCallGraph(LocalTUDeclsSize);
 
   // After all decls handled, run checkers on the entire TranslationUnit.
   checkerMgr->runCheckersOnEndOfTranslationUnit(TU, *Mgr, BR);
 
   BR.FlushReports();
   RecVisitorBR = nullptr;
+
+  // If the user wanted to analyze a specific function and the number of basic
+  // blocks analyzed is zero, than the user might not specified the function
+  // name correctly.
+  // FIXME: The user might have analyzed the requested function in Syntax mode,
+  // but we are unaware of that.
+  if (!Opts->AnalyzeSpecificFunction.empty() && NumFunctionsAnalyzed == 0)
+    reportAnalyzerFunctionMisuse(*Opts, *Ctx);
 }
 
 void AnalysisConsumer::reportAnalyzerProgress(StringRef S) {
   if (Opts->AnalyzerDisplayProgress)
     llvm::errs() << S;
 }
 
 void AnalysisConsumer::HandleTranslationUnit(ASTContext &C) {

clang/test/Analysis/analyze-function-guide.cpp

+int fizzbuzz(int x, bool y) {
+  return x + y;
+}
+
+// C++ but not uses parentheses in the '-analyze-function' option.
+//
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:   -analyze-function='missing_fn' -x c++ \
+// RUN:   2>&1 %s | FileCheck %s -check-prefix=CHECK-CXX
+//
+// CHECK-CXX:      Every top-level functions were skipped.
+// CHECK-CXX-NEXT: Pass the -analyzer-display-progress for tracking which functions are analyzed.
+// CHECK-CXX-NEXT: For analyzing C++ code you need to pass the function parameter list: -analyze-function="foobar(int, _Bool)"
+
+// C but uses parentheses in the '-analyze-function' option.
+//
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:   -analyze-function='missing_fn()' -x c -Dbool=_Bool \
+// RUN:   2>&1 %s | FileCheck %s -check-prefix=CHECK-C
+//
+// CHECK-C:      Every top-level functions were skipped.
+// CHECK-C-NEXT: Pass the -analyzer-display-progress for tracking which functions are analyzed.
+// CHECK-C-NEXT: For analyzing C code you shouldn't pass the function parameter list, only the name of the function: -analyze-function=foobar
+
+// The user passed the '-analyzer-display-progress' option, we don't need to advocate it.
+//
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:   -analyze-function=missing_fn \
+// RUN:   -analyzer-display-progress -x c -Dbool=_Bool \
+// RUN:   2>&1 %s | FileCheck %s -check-prefix=CHECK-DONT-ADVOCATE-DISPLAY-PROGRESS
+//
+// CHECK-DONT-ADVOCATE-DISPLAY-PROGRESS:     Every top-level functions were skipped.
+// CHECK-DONT-ADVOCATE-DISPLAY-PROGRESS-NOT: Pass the -analyzer-display-progress
+
+// The user passed the '-analyze-function' option but that doesn't mach to any declaration.
+//
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:   -analyze-function='missing_fn()' -x c++ \
+// RUN:   2>&1 %s | FileCheck %s -check-prefix=CHECK-ADVOCATE-DISPLAY-PROGRESS
+//
+// CHECK-ADVOCATE-DISPLAY-PROGRESS:      Every top-level functions were skipped.
+// CHECK-ADVOCATE-DISPLAY-PROGRESS-NEXT: Pass the -analyzer-display-progress for tracking which functions are analyzed.
+// CHECK-ADVOCATE-DISPLAY-PROGRESS-NOT:  For analyzing
+
+
+// The user passed the '-analyze-function' option and that matches on a declaration.
+//
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:   -analyze-function='fizzbuzz(int, _Bool)' -x c++ \
+// RUN:   2>&1 %s | FileCheck %s -check-prefix=CHECK-EMPTY --allow-empty
+//
+// Expected empty standard output.
+// CHECK-EMPTY-NOT: Every top-level functions were skipped.
+
+// Same as the previous but syntax mode only.
+// FIXME: This should have empty standard output.
+//
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config ipa=none \
+// RUN:   -analyze-function='fizzbuzz(int, _Bool)' -x c++ \
+// RUN:   2>&1 %s | FileCheck %s -check-prefix=CHECK-EMPTY2 --allow-empty
+//
+// FIXME: This should have empty standard output.
+// CHECK-EMPTY2:      Every top-level functions were skipped.
+// CHECK-EMPTY2-NEXT: Pass the -analyzer-display-progress for tracking which functions are analyzed.