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@-3{{TRUE}}
-#endif
+ // expected-warning@-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@-3{{TRUE}}
-#else
- // expected-warning@-5{{UNKNOWN}}
-#endif
+ // expected-warning@-2{{TRUE}}
#else
- // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-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;
}
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@-3{{TRUE}}
-#endif
+ // expected-warning@-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@-3{{TRUE}}
-#else
- // expected-warning@-5{{UNKNOWN}}
-#endif
+ // expected-warning@-2{{TRUE}}
#else
- // expected-warning@-8{{UNKNOWN}}
+ // expected-warning@-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;
}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
