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<MethodFault>& fault)
747 {
748    MethodFault::Exception *mfe = dynamic_cast<MethodFault::Exception *>(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<MethodFault>& fault)
747 {
748    ASSERT(e != nullptr);
749    MethodFault::Exception *mfe = dynamic_cast<MethodFault::Exception *>(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<MethodFault>& fault)
747 {
748    ASSERT(e != nullptr);
749    MethodFault::Exception *mfe = dynamic_cast<MethodFault::Exception *>(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<MethodFault>& fault)
748 {
749    ASSERT(e != nullptr);
750    MethodFault::Exception *mfe = dynamic_cast<MethodFault::Exception *>(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<MethodFault>& fault)
747 {
748    MethodFault::Exception *mfe = dynamic_cast<MethodFault::Exception *>(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 28671d45d01375effb18e3aa79c8a8266ae37f9d..5fbdf69a2261902ebc703fe9241f45bbb1c00c5c 100644 (file)
--- 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