[PATCH] D80286: [Analyzer] Allow creation of stack frame for functions without definition

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: NoQ, Szelethus.
baloghadamsoftware added a project: clang.
Herald added subscribers: ASDenysPetrov, martong, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, whisperity.
baloghadamsoftware added a parent revision: D79704: [Analyzer] [NFC] Parameter Regions.
baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added a comment.

I tested this on several open-source projects: BitCoin, CURL, OpenSLL, PostGreS, TMux, Xerces and even on LLVM itself with most of the projects enabled. No new crash and no change in findings. So it seems to be stable.



================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:179
-  // situation because there's a loss of precision anyway because we cannot
-  // inline the call.
-  RuntimeDefinition RD = getRuntimeDefinition();
----------------
We introduced `ParamRegion`s to overcome this, but please provide me the tests that crash when deleting these lines without `ParamRegions` you mentioned D49443#1193290.


================
Comment at: clang/test/Analysis/temporaries.cpp:893
     glob = 1;
-    // FIXME: Why is destructor not inlined in C++17
     clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
----------------
I wonder whether `clang_analyzer_checkInlined()` works correctly with this patch: it seems it only checks for stack frame which now any function with definition can have.


Retrieving the parameter location of functions was disabled because it may causes crashes due to the fact that functions may have multiple declarations and without definition it is difficult to ensure that always the same declration is used. Now parameters are stored in `ParamRegions` which are independent of the declaration of the function, therefore the same parameters always have the same regions, independently of the function declaration used actually. This allows us to remove the limitation described above.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80286

Files:
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/test/Analysis/explain-svals.cpp
  clang/test/Analysis/temporaries.cpp


Index: clang/test/Analysis/temporaries.cpp
===================================================================
--- clang/test/Analysis/temporaries.cpp
+++ clang/test/Analysis/temporaries.cpp
@@ -890,12 +890,9 @@
 public:
   ~C() {
     glob = 1;
-    // FIXME: Why is destructor not inlined in C++17
     clang_analyzer_checkInlined(true);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-    // expected-warning at -3{{TRUE}}
-#endif
+    // expected-warning at -2{{TRUE}}
 #endif
   }
 };
@@ -914,16 +911,11 @@
   // temporaries returned from functions, so we took the wrong branch.
   coin && is(get()); // no-crash
   if (coin) {
-    // FIXME: Why is destructor not inlined in C++17
     clang_analyzer_eval(glob);
 #ifdef TEMPORARY_DTORS
-#if __cplusplus < 201703L
-    // expected-warning at -3{{TRUE}}
-#else
-    // expected-warning at -5{{UNKNOWN}}
-#endif
+    // expected-warning at -2{{TRUE}}
 #else
-    // expected-warning at -8{{UNKNOWN}}
+    // expected-warning at -4{{UNKNOWN}}
 #endif
   } else {
     // The destructor is not called on this branch.
Index: clang/test/Analysis/explain-svals.cpp
===================================================================
--- clang/test/Analysis/explain-svals.cpp
+++ clang/test/Analysis/explain-svals.cpp
@@ -94,7 +94,7 @@
 } // end of anonymous namespace
 
 void test_6() {
-  clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$}}}}
+  clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of parameter 0 of function 'clang_analyzer_explain\(\)'$}}}}
   clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}}
 }
 
Index: clang/lib/StaticAnalyzer/Core/CallEvent.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -172,23 +172,9 @@
   if (!D)
     return nullptr;
 
-  // TODO: For now we skip functions without definitions, even if we have
-  // our own getDecl(), because it's hard to find out which re-declaration
-  // is going to be used, and usually clients don't really care about this
-  // situation because there's a loss of precision anyway because we cannot
-  // inline the call.
-  RuntimeDefinition RD = getRuntimeDefinition();
-  if (!RD.getDecl())
-    return nullptr;
-
   AnalysisDeclContext *ADC =
       LCtx->getAnalysisDeclContext()->getManager()->getContext(D);
 
-  // TODO: For now we skip virtual functions, because this also rises
-  // the problem of which decl to use, but now it's across different classes.
-  if (RD.mayHaveOtherDefinitions() || RD.getDecl() != ADC->getDecl())
-    return nullptr;
-
   return ADC;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D80286.265196.patch
Type: text/x-patch
Size: 2930 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200520/02202b3d/attachment.bin>