[Static Analyzer] Fix Analysis being skipped for code with declarations in .h file

[karthikthecool]

Summary

Hi Anna,
This patch fixes a regression introduced by r224398. Prior to r224398 (e.g. in llvm 3.5) we were able to analyze the following code in test-include.c and report a null deref in this case. But post r224398 this analysis is being skipped as a result we miss many bugs in our codebase.
E.g.

// test-include.c
#include "test-include.h"
void test(int * data) {
  data = 0;
  *data = 1;
}

 // test-include.h
void test(int * data);

This patch checks if the function declaration has a body in the mainfile currently being analyzed if yes then RunPathSensitiveChecks on the same. This fixes the above issue without any regressions.

Please let me know if you feel this patch looks good to you or if you feel there is a better way to fix the same.
Thanks and Regards
Karthik Bhat

[karthikthecool]

gentle ping..

[zaks.anna]

Sorry about the breakage.

What about computing SL based on the location of the body before any of the checks? Specifically, changing this line:

SourceLocation SL = SM.getExpansionLoc(D->getLocation());

Anna.

[karthikthecool]

Hi Anna,
Thanks for the review. Please find the updated code as per review comments.
Please let me know if this looks good to you.
Thanks and Regards
Karthik Bhat

[zaks.anna]

I think this should be all that's needed:

   // - System headers: don't run any checks.
   SourceManager &SM = Ctx->getSourceManager();
-  SourceLocation SL = SM.getExpansionLoc(D->getLocation());
+  
+  SourceLocation SL = D->hasBody() ? D->getBody()->getLocStart() 
+                                   : D->getLocation(); 
+  SL = SM.getExpansionLoc(SL);
+  
   if (!Opts->AnalyzeAll && !SM.isWrittenInMainFile(SL)) {

There are 2 differences from your patch:

1. I am not sure why the second if statement is added in your patch.
2. Getting the ExpansionLoc for the body. (In case it's a macro, it won't get analyzed.) Would be great if you add the macro test case as well.

Thanks!
Anna.

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

603

I don't think this if-statement is needed.

[karthikthecool]

Hi Anna,
Thanks for the input. Your code seems much more structured. Updated the code and tests as per your comments.
I had added-

if (SM.isInMainFile(SL))
  return Mode;

because my understanding was we do not analyze header files till analyze-all option is specified but it seems i was wrong we do analyze function definition inside header files if called from the main file(checked in llvm 3.5). Updated the code to remove the check.

Please let me know if you have any other comments on this patch.
Thanks for your time.

Regards
Karthik Bhat

[zaks.anna]

Committed in r240800.

Thank you VERY MUCH for sending in this patch. It fixes a truly bad regression!

Anna.

[karthikthecool]

Thanks Anna. (Has been committed as r240800)

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

AnalysisConsumer::getModeForDecl(Decl *D, AnalysisMode Mode) {
 
   // Unless -analyze-all is specified, treat decls differently depending on
   // where they came from:
   // - 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();
   SourceLocation SL = SM.getExpansionLoc(D->getLocation());
+  
+  // Check if the definition of the function declaration has a body.
+  // Update the location if function body is found.
+  if (D->hasBody())
+    SL = D->getBody()->getLocStart();
+
   if (!Opts->AnalyzeAll && !SM.isWrittenInMainFile(SL)) {
     if (SL.isInvalid() || SM.isInSystemHeader(SL))
       return AM_None;
+    // Return the current analysis mode if the location is in the main file.
+    if (SM.isInMainFile(SL))
+      return Mode;
     return Mode & ~AM_Path;
   }
 
   return Mode;
 }
 
 void AnalysisConsumer::HandleCode(Decl *D, AnalysisMode Mode,
                                   ExprEngine::InliningModes IMode,

test/Analysis/test-include.h

+void test(int * data);

test/Analysis/test-include.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
+#include "test-include.h"
+void test(int * data) {
+  data = 0;
+  *data = 1; // expected-warning{{Dereference of null pointer}}
+}