From: Oliver Kurth Date: Thu, 5 Dec 2019 19:34:43 +0000 (-0800) Subject: Introduce Clang SA-specific assert X-Git-Tag: stable-11.1.0~122 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=73454a5402f71520fefe38e207d03d13e84aca7a;p=thirdparty%2Fopen-vm-tools.git Introduce Clang SA-specific assert Introduce an assert which would only be generated during clang SA execution. With it we would avoid generating extra code from assertations which might be needed specifically for Clang SA's correct working (even with enabled analyzer, binary wouldn't grow, as __clang_analyzer__ is only defined while the analyzer is parsing the files for analysis, and not during Clang's compilation). Cases in which might be used is for example before a statement which we are fine just to fail or to possibly silence a false positive. A real example where we would require such assertion is for the attached report in the review. There seem to be a bug with the analyzer where it will falsely ignore branches if we assert a parent class pointer before using dynamic_cast as it will evaluate both pointers to be same. To show more precisely, we would use helper functions from debug checker ExprInspection. Example 1: 745 void 746 DvsKeeper::GetMethodFault(Exception *e, Ref& fault) 747 { 748 MethodFault::Exception *mfe = dynamic_cast(e); 749 clang_analyzer_warnIfReached(); 750 if (mfe != nullptr) { 751 clang_analyzer_warnIfReached(); 752 fault = mfe->GetFault(); 753 } else { 754 clang_analyzer_warnIfReached(); 755 fault = new SystemError(e->what()); 756 } 757 clang_analyzer_warnIfReached(); 758 } This would generate the following warnings: bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:749:4: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:751:7: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:754:7: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:755:31: warning: Called C++ object pointer is null fault = new SystemError(e->what()); ^~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:757:4: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 5 warnings generated. Example 2: 745 void 746 DvsKeeper::GetMethodFault(Exception *e, Ref& fault) 747 { 748 ASSERT(e != nullptr); 749 MethodFault::Exception *mfe = dynamic_cast(e); 750 clang_analyzer_warnIfReached(); 751 if (mfe != nullptr) { 752 clang_analyzer_warnIfReached(); 753 fault = mfe->GetFault(); 754 } else { 755 clang_analyzer_warnIfReached(); 756 fault = new SystemError(e->what()); 757 } 758 clang_analyzer_warnIfReached(); 759 } Result: bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:750:4: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:752:7: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:758:4: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3 warnings generated. When we do that, the whole else branch is unreachable by the analyzer, which shouldn't be the case. Example 3: 745 void 746 DvsKeeper::GetMethodFault(Exception *e, Ref& fault) 747 { 748 ASSERT(e != nullptr); 749 MethodFault::Exception *mfe = dynamic_cast(e); 750 ASSERT(mfe == nullptr); 751 clang_analyzer_warnIfReached(); 752 if (mfe != nullptr) { 753 clang_analyzer_warnIfReached(); 754 fault = mfe->GetFault(); 755 } else { 756 clang_analyzer_warnIfReached(); 757 fault = new SystemError(e->what()); 758 } 759 clang_analyzer_warnIfReached(); 760 } No warnings are generated, and 'mfe' can be null if the cast fails. Example 4: 746 void 747 DvsKeeper::GetMethodFault(Exception *e, Ref& fault) 748 { 749 ASSERT(e != nullptr); 750 MethodFault::Exception *mfe = dynamic_cast(e); 751 clang_analyzer_eval(e == mfe); 752 clang_analyzer_warnIfReached(); 753 if (mfe != nullptr) { 754 clang_analyzer_warnIfReached(); 755 fault = mfe->GetFault(); 756 } else { 757 clang_analyzer_warnIfReached(); 758 fault = new SystemError(e->what()); 759 } 760 clang_analyzer_warnIfReached(); 761 } bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:751:4: warning: TRUE clang_analyzer_eval(e == mfe); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:752:4: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:754:7: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:760:4: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 4 warnings generated. Even knowing 'e' is not a nullptr, the analyzer falsely evaluates 'e' and 'mfe' to be the same, which leads to many other wrong assumptions. Example 5 (What would be best way to work around this bug with least analyzer impact): 745 void 746 DvsKeeper::GetMethodFault(Exception *e, Ref& fault) 747 { 748 MethodFault::Exception *mfe = dynamic_cast(e); 749 clang_analyzer_warnIfReached(); 750 if (mfe != nullptr) { 751 clang_analyzer_warnIfReached(); 752 fault = mfe->GetFault(); 753 } else { 754 clang_analyzer_warnIfReached(); 755 ASSERT(e != nullptr); 756 clang_analyzer_warnIfReached(); 757 fault = new SystemError(e->what()); 758 } 759 clang_analyzer_warnIfReached(); 760 } bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:749:4: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:751:7: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:754:7: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ bora/vpx/vpxd/dvs/core/dvsKeeper.cpp:759:4: warning: REACHABLE clang_analyzer_warnIfReached(); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 4 warnings generated. In such cases, we could look into the problem as both real and analyzer issue and other ways to "fix" it either don't work, or impact the further work of the analyzer, which we would like to be as minimal as possible. --- diff --git a/open-vm-tools/lib/include/vm_assert.h b/open-vm-tools/lib/include/vm_assert.h index 28671d45d..5fbdf69a2 100644 --- a/open-vm-tools/lib/include/vm_assert.h +++ b/open-vm-tools/lib/include/vm_assert.h @@ -353,6 +353,17 @@ void WarningThrottled(uint32 *count, const char *fmt, ...) PRINTF_DECL(2, 3); assertions \ } +/* + * Avoid generating extra code due to asserts which are required by + * Clang static analyzer, e.g. right before a statement would fail, using + * the __clang_analyzer__ macro defined only when clang SA is parsing files. + */ +#ifdef __clang_analyzer__ +#define ANALYZER_ASSERT(cond) ASSERT(cond) +#else +#define ANALYZER_ASSERT(cond) ((void)0) +#endif + #ifdef __cplusplus } /* extern "C" */ #endif