Page MenuHomePhabricator

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

Authored by karthikthecool on Jun 1 2015, 12:33 AM.

Details

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

Diff Detail

Event Timeline

karthikthecool retitled this revision from to [Static Analyzer] Fix Analysis being skipped for code with declarations in .h file.
karthikthecool updated this object.
karthikthecool edited the test plan for this revision. (Show Details)
karthikthecool added reviewers: zaks.anna, klimek.
karthikthecool added a subscriber: Unknown Object (MLST).
zaks.anna edited edge metadata.Jun 22 2015, 3:47 PM

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 edited edge metadata.

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

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
600

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

dkrupp added a subscriber: dkrupp.Jun 26 2015, 12:29 AM

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

Committed in r240800.

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

Anna.

karthikthecool accepted this revision.Jun 26 2015, 9:31 PM
karthikthecool added a reviewer: karthikthecool.
This revision is now accepted and ready to land.Jun 26 2015, 9:31 PM

Thanks Anna. (Has been committed as r240800)