[analyzer] Add support for WebKit "unified sources".

[NoQ]

Summary

Since recently, WebKit uses a peculiar build system that compiles multiple translation units at once by automatically joining them into a bigger file via #include. Because none of the functions end up in the main file, we disable all our path-sensitive checks on such unified sources. Try to work around that.

[NoQ]

Hopefully a more portable way of testing this stuff.

[baloghadamsoftware]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

148

Although not very common, but .cxx is also a possibly extension for C++ source files.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

837

Maybe we should include a test for container methods as well.

[george.karpenkov]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

144

Is this if really necessary? This logic has too much overfitting, and it seems that if someone decides to include .cc files, we should analyze them in any case, right? We also would prefer to not stop working if webkit decides on using a different naming for those.

lib/StaticAnalyzer/Core/CallEvent.cpp

975

You would shave off a bit of redundancy and verbosity by saving AnalysisManager in a local variable.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

837

+1

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

152–153

Assert error message is not quite technically correct now

test/Analysis/unified-sources/source1.cpp

9

Wow, expected-* directives work across multiple files?? This is really cool!

[NoQ]

Address most comments.

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

144

This is indeed an act of overfitting. But also there are very few reasons to include a non-header file, and all of them are pretty exotic. I'm not sure we want to analyze these files in all cases. So i want to play safe until we gather more data.

148

Yup, thanks!

test/Analysis/unified-sources/source1.cpp

9

-verify works over preprocessed files, so yeah, these directives respect #ifs (which we regularly abuse) and #includes (which we rarely abuse). This test also acts as a normal test, but the UnifiedSource-1.cpp test is the one that'll fail if we break the newly added functionality.

[majnemer]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

145–147

C++ source code is also found in files which end in .C, this code will match against strange file endings like .cXx and .mM

I think the above logic should be changed to match https://github.com/llvm-mirror/clang/blob/master/lib/Frontend/FrontendOptions.cpp#L27

[NoQ]

A more accurate extension check.

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

145–147

Aha, yeah, thanks, that's the place i was looking for.

[george.karpenkov]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

144

I would still say that just analyzing included c++ files is a lesser evil.

145–147

Why not just use the included function then? It's static.

[NoQ]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

145–147

Because it has a different purpose:

for now it doesn't discriminate between code and header files

[probinson]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

144

Agreed. WebKit is not the only project that does this kind of thing.

[NoQ]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

144

The question is, are all such projects in fact interested in having the analyzer analyze the respective code?

For instance, if the included code is autogenerated by an external tool, users are probably not interested in finding bugs in such code because they will be unable to fix them.

Are you interested in providing a list of examples of projects that do this kind of thing, explain why are they doing it, and whether they will be interested in having warnings in included files?

[NoQ]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

144

Note that we're not talking about warnings that are *emitted* in the included file. We're talking about warnings that *originate* within the included file (and as such most likely stay within the included file and its includes). This makes a difference because we are doing inter-procedural analysis. I.e., if we find that passing a null pointer in the main file into a header function causes a null dereference in the header function, the report that originates within the main file will be emitted in the header with auxiliary path notes in the main file.

So if a small chunk of code is included but the main file still contains reasonable stuff, we'll be able to find most bugs in the included code as it is being used from the rest of the file. The problem only arises when all code is included.

[a.sidorin]

Hi Artem. Cool patch!

Maybe we should create an analyzer option for this? So, all users who want webkit-like behaviour will just enable (or disable, don't know what's better) the option and get the feature. I know that introducing a new option is undesirable but it seems to be a good compromise here.

[probinson]

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

144

Game teams are known to use "unity" builds, developing their code in separate files then having a master .cpp that will #include everything in a given library/component. This separates source into reasonable sized and logically coherent units, but the unity build improves build time for these large projects. If a team is using static analysis, they will want it on the entire compilation.

[NoQ]

Aha, ok, yeah, that sounds like a lot, thank you. I think i'll follow up with a separate commit that will enable first-level-code-file-include analysis in all files under an on-by-default -analyzer-config flag, would that make sense?

[probinson]

In D45839#1077258, @NoQ wrote:

Aha, ok, yeah, that sounds like a lot, thank you. I think i'll follow up with a separate commit that will enable first-level-code-file-include analysis in all files under an on-by-default -analyzer-config flag, would that make sense?

Works for me, thanks!

include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h

public:
 
   ParentMap &getParentMap(Decl const *D) {
     return AnaCtxMgr.getContext(D)->getParentMap();
   }
 
   AnalysisDeclContext *getAnalysisDeclContext(const Decl *D) {
     return AnaCtxMgr.getContext(D);
   }
+
+  static bool isInCodeFile(SourceLocation SL, const SourceManager &SM) {
+    if (SM.isInMainFile(SL))
+      return true;
+
+    // Support the "unified sources" compilation method (eg. WebKit) that
+    // involves producing non-header files that include other non-header files.
+    // We should be included directly from a UnifiedSource* file
+    // and we shouldn't be a header - which is a very safe defensive check.
+    SourceLocation IL = SM.getIncludeLoc(SM.getFileID(SL));
+    if (!IL.isValid() || !SM.isInMainFile(IL))
+      return false;
+    // Should rather be "file name starts with", but the current .getFilename
+    // includes the full path.
+    if (SM.getFilename(IL).contains("UnifiedSource")) {
+      // It might be great to reuse FrontendOptions::getInputKindForExtension()
+      // but for now it doesn't discriminate between code and header files.
+      return llvm::StringSwitch<bool>(SM.getFilename(SL).rsplit('.').second)
+          .Cases("c", "m", "mm", "C", "cc", "cp", true)
+          .Cases("cpp", "CPP", "c++", "cxx", "cppm", true)
+          .Default(false);
+    }
+
+    return false;
+  }
+
+  bool isInCodeFile(SourceLocation SL) {
+    const SourceManager &SM = getASTContext().getSourceManager();
+    return isInCodeFile(SL, SM);
+  }
 };
 
 } // enAnaCtxMgrspace
 
 } // end clang namespace
 
 #endif

lib/StaticAnalyzer/Core/CallEvent.cpp

const ObjCPropertyDecl *ObjCMethodCall::getAccessedProperty() const {
 
   // Note: This is potentially quite slow.
   return MD->findPropertyDecl();
 }
 
 bool ObjCMethodCall::canBeOverridenInSubclass(ObjCInterfaceDecl *IDecl,
                                              Selector Sel) const {
   assert(IDecl);
-  const SourceManager &SM =
-    getState()->getStateManager().getContext().getSourceManager();
-
+  AnalysisManager &AMgr =
+      getState()->getStateManager().getOwningEngine()->getAnalysisManager();
   // If the class interface is declared inside the main file, assume it is not
   // subcassed.
   // TODO: It could actually be subclassed if the subclass is private as well.
   // This is probably very rare.
   SourceLocation InterfLoc = IDecl->getEndOfDefinitionLoc();
-  if (InterfLoc.isValid() && SM.isInMainFile(InterfLoc))
+  if (InterfLoc.isValid() && AMgr.isInCodeFile(InterfLoc))
     return false;
 
   // Assume that property accessors are not overridden.
   if (getMessageKind() == OCM_PropertyAccess)
     return false;
 
   // We assume that if the method is public (declared outside of main file) or
   // has a parent which publicly declares the method, the method could be
   // overridden in a subclass.
 
   // Find the first declaration in the class hierarchy that declares
   // the selector.
   ObjCMethodDecl *D = nullptr;
   while (true) {
     D = IDecl->lookupMethod(Sel, true);
 
     // Cannot find a public definition.
     if (!D)
       return false;
 
     // If outside the main file,
-    if (D->getLocation().isValid() && !SM.isInMainFile(D->getLocation()))
+    if (D->getLocation().isValid() && !AMgr.isInCodeFile(D->getLocation()))
       return true;
 
     if (D->isOverriding()) {
       // Search in the superclass on the next iteration.
       IDecl = D->getClassInterface();
       if (!IDecl)
         return false;
 
       IDecl = IDecl->getSuperClass();
       if (!IDecl)
         return false;
 

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

static bool isCXXSharedPtrDtor(const FunctionDecl *FD) {
   return false;
 }
 
 /// Returns true if the function in \p CalleeADC may be inlined in general.
 ///
 /// This checks static properties of the function, such as its signature and
 /// CFG, to determine whether the analyzer should ever consider inlining it,
 /// in any context.
-static bool mayInlineDecl(AnalysisDeclContext *CalleeADC,
-                          AnalyzerOptions &Opts) {
+static bool mayInlineDecl(AnalysisManager &AMgr,
+                          AnalysisDeclContext *CalleeADC) {
+  AnalyzerOptions &Opts = AMgr.getAnalyzerOptions();
   // FIXME: Do not inline variadic calls.
   if (CallEvent::isVariadic(CalleeADC->getDecl()))
     return false;
 
   // Check certain C++-related inlining policies.
   ASTContext &Ctx = CalleeADC->getASTContext();
   if (Ctx.getLangOpts().CPlusPlus) {
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(CalleeADC->getDecl())) {
       // Conditionally control the inlining of template functions.
       if (!Opts.mayInlineTemplateFunctions())
         if (FD->getTemplatedKind() != FunctionDecl::TK_NonTemplate)
           return false;
 
       // Conditionally control the inlining of C++ standard library functions.
       if (!Opts.mayInlineCXXStandardLibrary())
         if (Ctx.getSourceManager().isInSystemHeader(FD->getLocation()))
           if (AnalysisDeclContext::isInStdNamespace(FD))
             return false;
 
       // Conditionally control the inlining of methods on objects that look
       // like C++ containers.
       if (!Opts.mayInlineCXXContainerMethods())
-        if (!Ctx.getSourceManager().isInMainFile(FD->getLocation()))
+        if (!AMgr.isInCodeFile(FD->getLocation()))
           if (isContainerMethod(Ctx, FD))
             return false;
 
       // Conditionally control the inlining of the destructor of C++ shared_ptr.
       // We don't currently do a good job modeling shared_ptr because we can't
       // see the reference count, so treating as opaque is probably the best
       // idea.
       if (!Opts.mayInlineCXXSharedPtrDtor())
         if (isCXXSharedPtrDtor(FD))
           return false;
     }
bool ExprEngine::shouldInlineCall(const CallEvent &Call, const Decl *D,
   Optional<bool> MayInline = Engine.FunctionSummaries->mayInline(D);
   if (MayInline.hasValue()) {
     if (!MayInline.getValue())
       return false;
 
   } else {
     // We haven't actually checked the static properties of this function yet.
     // Do that now, and record our decision in the function summaries.
-    if (mayInlineDecl(CalleeADC, Opts)) {
+    if (mayInlineDecl(getAnalysisManager(), CalleeADC)) {
       Engine.FunctionSummaries->markMayInline(D);
     } else {
       Engine.FunctionSummaries->markShouldNotInline(D);
       return false;
     }
   }
 
   // Check if we should inline a call based on its kind.

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

 #include "clang/AST/Type.h"
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"

 getFirstStackedCallToHeaderFile(PathDiagnosticCallPiece *CP,
                                 const SourceManager &SMgr) {
   SourceLocation CallLoc = CP->callEnter.asLocation();
 
   // If the call is within a macro, don't do anything (for now).
   if (CallLoc.isMacroID())
     return nullptr;
 
-  assert(SMgr.isInMainFile(CallLoc) &&
-         "The call piece should be in the main file.");
+  assert(AnalysisManager::isInCodeFile(CallLoc, SMgr) &&
+         "The call piece should not be in a header file.");
 
   // Check if CP represents a path through a function outside of the main file.
-  if (!SMgr.isInMainFile(CP->callEnterWithin.asLocation()))
+  if (!AnalysisManager::isInCodeFile(CP->callEnterWithin.asLocation(), SMgr))
     return CP;
 
   const PathPieces &Path = CP->path;
   if (Path.empty())
     return nullptr;
 
   // Check if the last piece in the callee path is a call to a function outside
   // of the main file.

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

AnalysisConsumer::getModeForDecl(Decl *D, AnalysisMode Mode) {
   // - Main source file: run both path-sensitive and non-path-sensitive checks.
   // - Header files: run non-path-sensitive checks only.
   // - System headers: don't run any checks.
   SourceManager &SM = Ctx->getSourceManager();
   const Stmt *Body = D->getBody();
   SourceLocation SL = Body ? Body->getLocStart() : D->getLocation();
   SL = SM.getExpansionLoc(SL);
 
-  if (!Opts->AnalyzeAll && !SM.isWrittenInMainFile(SL)) {
+  if (!Opts->AnalyzeAll && !Mgr->isInCodeFile(SL)) {
     if (SL.isInvalid() || SM.isInSystemHeader(SL))
       return AM_None;
     return Mode & ~AM_Path;
   }
 
   return Mode;
 }
 

test/Analysis/unified-sources/UnifiedSource-1.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// There should still be diagnostics within included files.
+#include "source1.cpp"
+#include "source2.cpp"

test/Analysis/unified-sources/container.h

+class ContainerInHeaderFile {
+  class Iterator {
+  };
+
+public:
+  Iterator begin() const;
+  Iterator end() const;
+
+  int method() { return 0; }
+};

test/Analysis/unified-sources/source1.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// This test tests that the warning is here when it is included from
+// the unified sources file. The run-line in this file is there
+// only to suppress LIT warning for the complete lack of run-line.
+int foo(int x) {
+  if (x) {}
+  return 1 / x; // expected-warning{{}}
+}
+
+// Let's see if the container inlining heuristic still works.
+#include "container.h"
+int testContainerMethodInHeaderFile(ContainerInHeaderFile Cont) {
+  return 1 / Cont.method(); // no-warning
+}

test/Analysis/unified-sources/source2.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// This test tests that the warning is here when it is included from
+// the unified sources file. The run-line in this file is there
+// only to suppress LIT warning for the complete lack of run-line.
+int testNullDereference() {
+  int *x = 0;
+  return *x; // expected-warning{{}}
+}
+
+// Let's see if the container inlining heuristic still works.
+class ContainerInCodeFile {
+  class Iterator {
+  };
+
+public:
+  Iterator begin() const;
+  Iterator end() const;
+
+  int method() { return 0; }
+};
+
+int testContainerMethodInCodeFile(ContainerInCodeFile Cont) {
+  return 1 / Cont.method(); // expected-warning{{}}
+}