Correcting NO_ASSERT handling and safety. Fixes #1706, #2447 (#2448)

nasa · Jan 5, 2024 · 1caf1e4 · 1caf1e4
1 parent 5d68e1a
commit 1caf1e4
Show file tree

Hide file tree

Showing 9 changed files with 112 additions and 43 deletions.
diff --git a/Fw/Types/Assert.cpp b/Fw/Types/Assert.cpp
@@ -5,10 +5,6 @@
 
 #define FW_ASSERT_DFL_MSG_LEN 256
 
-#if FW_ASSERT_LEVEL == FW_NO_ASSERT
-
-#else
-
 #if FW_ASSERT_LEVEL == FW_FILEID_ASSERT
 #define fileIdFs "Assert: 0x%08" PRIx32 ":%" PRI_PlatformUIntType
 #else
@@ -287,6 +283,3 @@ NATIVE_INT_TYPE CAssert0(FILE_NAME_ARG file, NATIVE_UINT_TYPE lineNo) {
     }
     return 0;
 }
-
-#endif // FW_NO_ASSERT
-
diff --git a/Fw/Types/Assert.hpp b/Fw/Types/Assert.hpp
@@ -3,15 +3,20 @@
 
 #include <FpConfig.hpp>
 
-#if FW_ASSERT_LEVEL == FW_NO_ASSERT
-    #define FW_ASSERT(...)
-#else // ASSERT is defined
-
 // Return only the first argument passed to the macro.
 #define FW_ASSERT_FIRST_ARG(ARG_0, ...) ARG_0
 // Return all the arguments of the macro, but the first one
 #define FW_ASSERT_NO_FIRST_ARG(ARG_0, ...) __VA_ARGS__
 
+#if FW_ASSERT_LEVEL == FW_NO_ASSERT
+    // Users may override the NO_ASSERT case should they choose
+    #ifndef FW_ASSERT
+    #define FW_ASSERT(...) ((void)(FW_ASSERT_FIRST_ARG(__VA_ARGS__)))
+    #endif
+    #define FILE_NAME_ARG const CHAR*
+#else // ASSERT is defined
+
+
 // Passing the __LINE__ argument at the end of the function ensures that
 // the FW_ASSERT_NO_FIRST_ARG macro will never have an empty variadic variable
 #if FW_ASSERT_LEVEL == FW_FILEID_ASSERT
@@ -30,6 +35,7 @@
         ((void) ((FW_ASSERT_FIRST_ARG(__VA_ARGS__, 0)) ? (0) : \
         (Fw::SwAssert(__FILE__, FW_ASSERT_NO_FIRST_ARG(__VA_ARGS__, __LINE__)))))
 #endif
+#endif // if ASSERT is defined
 
 // F' Assertion functions can technically return even though the intention is for the assertion to terminate the program.
 // This breaks static analysis depending on assertions, since the analyzer has to assume the assertion will return.
@@ -142,7 +148,5 @@ namespace Fw {
             AssertHook *previousHook;
     };
 }
-#endif // if ASSERT is defined
-
 
 #endif // FW_ASSERT_HPP
diff --git a/Fw/Types/test/ut/TypesTest.cpp b/Fw/Types/test/ut/TypesTest.cpp
@@ -847,46 +847,72 @@ void AssertTest() {
 
     // issue an assert
     FW_ASSERT(0);
+#if FW_ASSERT_LEVEL != FW_NO_ASSERT
     // hook should have intercepted it
     ASSERT_TRUE(hook.asserted());
     ASSERT_EQ(0u,hook.getNumArgs());
+#else
+    // assert does not fire when asserts are off
+    ASSERT_FALSE(hook.asserted());
+#endif
 
     // issue an assert
     FW_ASSERT(0,1);
+#if FW_ASSERT_LEVEL != FW_NO_ASSERT
     // hook should have intercepted it
     ASSERT_TRUE(hook.asserted());
     ASSERT_EQ(1u,hook.getNumArgs());
     ASSERT_EQ(1u,hook.getArg1());
+#else
+    // assert does not fire when asserts are off
+    ASSERT_FALSE(hook.asserted());
+#endif
 
     // issue an assert
     FW_ASSERT(0,1,2);
+#if FW_ASSERT_LEVEL != FW_NO_ASSERT
     // hook should have intercepted it
     ASSERT_TRUE(hook.asserted());
     ASSERT_EQ(2u,hook.getNumArgs());
     ASSERT_EQ(1u,hook.getArg1());
     ASSERT_EQ(2u,hook.getArg2());
+#else
+    // assert does not fire when asserts are off
+    ASSERT_FALSE(hook.asserted());
+#endif
 
     // issue an assert
     FW_ASSERT(0,1,2,3);
+#if FW_ASSERT_LEVEL != FW_NO_ASSERT
     // hook should have intercepted it
     ASSERT_TRUE(hook.asserted());
     ASSERT_EQ(3u,hook.getNumArgs());
     ASSERT_EQ(1u,hook.getArg1());
     ASSERT_EQ(2u,hook.getArg2());
     ASSERT_EQ(3u,hook.getArg3());
+#else
+    // assert does not fire when asserts are off
+    ASSERT_FALSE(hook.asserted());
+#endif
 
     // issue an assert
     FW_ASSERT(0,1,2,3,4);
+#if FW_ASSERT_LEVEL != FW_NO_ASSERT
     // hook should have intercepted it
     ASSERT_TRUE(hook.asserted());
     ASSERT_EQ(4u,hook.getNumArgs());
     ASSERT_EQ(1u,hook.getArg1());
     ASSERT_EQ(2u,hook.getArg2());
     ASSERT_EQ(3u,hook.getArg3());
     ASSERT_EQ(4u,hook.getArg4());
+#else
+    // assert does not fire when asserts are off
+    ASSERT_FALSE(hook.asserted());
+#endif
 
     // issue an assert
     FW_ASSERT(0,1,2,3,4,5);
+#if FW_ASSERT_LEVEL != FW_NO_ASSERT
     // hook should have intercepted it
     ASSERT_TRUE(hook.asserted());
     ASSERT_EQ(5u,hook.getNumArgs());
@@ -895,9 +921,14 @@ void AssertTest() {
     ASSERT_EQ(3u,hook.getArg3());
     ASSERT_EQ(4u,hook.getArg4());
     ASSERT_EQ(5u,hook.getArg5());
+#else
+    // assert does not fire when asserts are off
+    ASSERT_FALSE(hook.asserted());
+#endif
 
     // issue an assert
     FW_ASSERT(0,1,2,3,4,5,6);
+#if FW_ASSERT_LEVEL != FW_NO_ASSERT
     // hook should have intercepted it
     ASSERT_TRUE(hook.asserted());
     ASSERT_EQ(6u,hook.getNumArgs());
@@ -907,6 +938,10 @@ void AssertTest() {
     ASSERT_EQ(4u,hook.getArg4());
     ASSERT_EQ(5u,hook.getArg5());
     ASSERT_EQ(6u,hook.getArg6());
+#else
+    // assert does not fire when asserts are off
+    ASSERT_FALSE(hook.asserted());
+#endif
 
 }
 

diff --git a/Os/Posix/IntervalTimer.cpp b/Os/Posix/IntervalTimer.cpp
@@ -14,8 +14,8 @@
 namespace Os {
     void IntervalTimer::getRawTime(RawTime& time) {
         timespec t;
-
-        FW_ASSERT(clock_gettime(CLOCK_REALTIME,&t) == 0,errno);
+        PlatformIntType status = clock_gettime(CLOCK_REALTIME,&t);
+        FW_ASSERT(status == 0,errno);
         time.upper = t.tv_sec;
         time.lower = t.tv_nsec;
     }

diff --git a/Ref/SendBuffApp/SendBuffComponentImpl.cpp b/Ref/SendBuffApp/SendBuffComponentImpl.cpp
@@ -52,7 +52,8 @@ namespace Ref {
             // reset buffer
             this->m_testBuff.resetSer();
             // serialize packet id
-            FW_ASSERT(this->m_testBuff.serialize(this->m_currPacketId) == Fw::FW_SERIALIZE_OK);
+            Fw::SerializeStatus serStat = this->m_testBuff.serialize(this->m_currPacketId);
+            FW_ASSERT(serStat == Fw::FW_SERIALIZE_OK);
             // increment packet id
             this->m_currPacketId++;
             this->m_buffsSent++;
@@ -75,9 +76,11 @@ namespace Ref {
                 this->log_WARNING_HI_PacketErrorInserted(this->m_currPacketId-1);
             }
             // serialize data
-            FW_ASSERT(this->m_testBuff.serialize(testData,dataSize) == Fw::FW_SERIALIZE_OK);
+            serStat = this->m_testBuff.serialize(testData,dataSize);
+            FW_ASSERT(serStat == Fw::FW_SERIALIZE_OK);
             // serialize checksum
-            FW_ASSERT(this->m_testBuff.serialize(csum) == Fw::FW_SERIALIZE_OK);
+            serStat = this->m_testBuff.serialize(csum);
+            FW_ASSERT(serStat == Fw::FW_SERIALIZE_OK);
             // send data
             this->Data_out(0,this->m_testBuff);
 

diff --git a/Svc/AssertFatalAdapter/test/ut/AssertFatalAdapterTester.cpp b/Svc/AssertFatalAdapter/test/ut/AssertFatalAdapterTester.cpp
@@ -42,6 +42,14 @@ namespace Svc {
         U32 lineNo;
         char file[80 + 1]; // Limit to 80  characters in the port call
         Fw::String fileString;
+
+// Asserts may be turned off resulting in this component doing a no-op
+#if FW_ASSERT_LEVEL == FW_NO_ASSERT
+        const int expectedSize = 0;
+#else
+        const int expectedSize = 1;
+#endif
+
 #if FW_ASSERT_LEVEL == FW_FILEID_ASSERT
         fileString.format("0x%08" PRIX32, ASSERT_FILE_ID);
 #else
@@ -52,45 +60,58 @@ namespace Svc {
         // FW_ASSERT_0
 
         FW_ASSERT(0);lineNo = __LINE__;
-        ASSERT_EVENTS_AF_ASSERT_0_SIZE(1);
-        ASSERT_EVENTS_AF_ASSERT_0(0,file,lineNo);
+        ASSERT_EVENTS_AF_ASSERT_0_SIZE(expectedSize);
+        if (expectedSize > 0) {
+            ASSERT_EVENTS_AF_ASSERT_0(0,file,lineNo);
+        }
 
         // FW_ASSERT_1
 
         FW_ASSERT(0,1);lineNo = __LINE__;
-        ASSERT_EVENTS_AF_ASSERT_1_SIZE(1);
-        ASSERT_EVENTS_AF_ASSERT_1(0,file,lineNo,1);
+        ASSERT_EVENTS_AF_ASSERT_1_SIZE(expectedSize);
+        if (expectedSize > 0) {
+            ASSERT_EVENTS_AF_ASSERT_1(0,file,lineNo,1);
+        }
 
         // FW_ASSERT_2
 
         FW_ASSERT(0,1,2);lineNo = __LINE__;
-        ASSERT_EVENTS_AF_ASSERT_2_SIZE(1);
-        ASSERT_EVENTS_AF_ASSERT_2(0,file,lineNo,1,2);
+        ASSERT_EVENTS_AF_ASSERT_2_SIZE(expectedSize);
+        if (expectedSize > 0) {
+            ASSERT_EVENTS_AF_ASSERT_2(0,file,lineNo,1,2);
+        }
 
         // FW_ASSERT_3
 
         FW_ASSERT(0,1,2,3);lineNo = __LINE__;
-        ASSERT_EVENTS_AF_ASSERT_3_SIZE(1);
-        ASSERT_EVENTS_AF_ASSERT_3(0,file,lineNo,1,2,3);
+        ASSERT_EVENTS_AF_ASSERT_3_SIZE(expectedSize);
+        if (expectedSize > 0) {
+             ASSERT_EVENTS_AF_ASSERT_3(0,file,lineNo,1,2,3);
+        }
 
         // FW_ASSERT_4
 
         FW_ASSERT(0,1,2,3,4);lineNo = __LINE__;
-        ASSERT_EVENTS_AF_ASSERT_4_SIZE(1);
-        ASSERT_EVENTS_AF_ASSERT_4(0,file,lineNo,1,2,3,4);
+        ASSERT_EVENTS_AF_ASSERT_4_SIZE(expectedSize);
+        if (expectedSize > 0) {
+            ASSERT_EVENTS_AF_ASSERT_4(0,file,lineNo,1,2,3,4);
+        }
 
         // FW_ASSERT_5
 
         FW_ASSERT(0,1,2,3,4,5);lineNo = __LINE__;
-        ASSERT_EVENTS_AF_ASSERT_5_SIZE(1);
-        ASSERT_EVENTS_AF_ASSERT_5(0,file,lineNo,1,2,3,4,5);
+        ASSERT_EVENTS_AF_ASSERT_5_SIZE(expectedSize);
+        if (expectedSize > 0) {
+             ASSERT_EVENTS_AF_ASSERT_5(0,file,lineNo,1,2,3,4,5);
+        }
 
         // FW_ASSERT_6
 
         FW_ASSERT(0,1,2,3,4,5,6);lineNo = __LINE__;
-        ASSERT_EVENTS_AF_ASSERT_6_SIZE(1);
-        ASSERT_EVENTS_AF_ASSERT_6(0,file,lineNo,1,2,3,4,5,6);
-
+        ASSERT_EVENTS_AF_ASSERT_6_SIZE(expectedSize);
+        if (expectedSize > 0) {
+            ASSERT_EVENTS_AF_ASSERT_6(0,file,lineNo,1,2,3,4,5,6);
+        }
         // Test unexpected assert
 #if FW_ASSERT_LEVEL == FW_FILEID_ASSERT
         U32 unexpectedFile = 0xF00;

diff --git a/Svc/GenericHub/GenericHubComponentImpl.cpp b/Svc/GenericHub/GenericHubComponentImpl.cpp
@@ -147,13 +147,18 @@ void GenericHubComponentImpl ::LogRecv_handler(const NATIVE_INT_TYPE portNum,
                                   Fw::Time& timeTag,
                                   const Fw::LogSeverity& severity,
                                   Fw::LogBuffer& args) {
+    Fw::SerializeStatus status = Fw::FW_SERIALIZE_OK;
     U8 buffer[sizeof(FwEventIdType) + Fw::Time::SERIALIZED_SIZE + Fw::LogSeverity::SERIALIZED_SIZE + FW_LOG_BUFFER_MAX_SIZE];
     Fw::ExternalSerializeBuffer serializer(buffer, sizeof(buffer));
     serializer.resetSer();
-    FW_ASSERT(serializer.serialize(id) == Fw::SerializeStatus::FW_SERIALIZE_OK);;
-    FW_ASSERT(serializer.serialize(timeTag) == Fw::SerializeStatus::FW_SERIALIZE_OK);
-    FW_ASSERT(serializer.serialize(severity) == Fw::SerializeStatus::FW_SERIALIZE_OK);
-    FW_ASSERT(serializer.serialize(args) == Fw::SerializeStatus::FW_SERIALIZE_OK);
+    status = serializer.serialize(id);
+    FW_ASSERT(status == Fw::SerializeStatus::FW_SERIALIZE_OK);
+    status = serializer.serialize(timeTag);
+    FW_ASSERT(status == Fw::SerializeStatus::FW_SERIALIZE_OK);
+    status = serializer.serialize(severity);
+    FW_ASSERT(status == Fw::SerializeStatus::FW_SERIALIZE_OK);
+    status = serializer.serialize(args);
+    FW_ASSERT(status == Fw::SerializeStatus::FW_SERIALIZE_OK);
     U32 size = serializer.getBuffLength();
     this->send_data(HubType::HUB_TYPE_EVENT, portNum, buffer, size);
 
@@ -163,12 +168,16 @@ void GenericHubComponentImpl ::TlmRecv_handler(const NATIVE_INT_TYPE portNum,
                                   FwChanIdType id,
                                   Fw::Time& timeTag,
                                   Fw::TlmBuffer& val) {
+    Fw::SerializeStatus status = Fw::FW_SERIALIZE_OK;
     U8 buffer[sizeof(FwChanIdType) + Fw::Time::SERIALIZED_SIZE + FW_TLM_BUFFER_MAX_SIZE];
     Fw::ExternalSerializeBuffer serializer(buffer, sizeof(buffer));
     serializer.resetSer();
-    FW_ASSERT(serializer.serialize(id) == Fw::SerializeStatus::FW_SERIALIZE_OK);
-    FW_ASSERT(serializer.serialize(timeTag) == Fw::SerializeStatus::FW_SERIALIZE_OK);
-    FW_ASSERT(serializer.serialize(val) == Fw::SerializeStatus::FW_SERIALIZE_OK);
+    status = serializer.serialize(id);
+    FW_ASSERT(status == Fw::SerializeStatus::FW_SERIALIZE_OK);
+    status = serializer.serialize(timeTag);
+    FW_ASSERT(status == Fw::SerializeStatus::FW_SERIALIZE_OK);
+    status = serializer.serialize(val);
+    FW_ASSERT(status == Fw::SerializeStatus::FW_SERIALIZE_OK);
     U32 size = serializer.getBuffLength();
     this->send_data(HubType::HUB_TYPE_CHANNEL, portNum, buffer, size);
 }

diff --git a/Svc/PrmDb/PrmDbImpl.cpp b/Svc/PrmDb/PrmDbImpl.cpp
@@ -308,7 +308,8 @@ namespace Svc {
             // reset deserialization
             buff.resetDeser();
             // deserialize, since record size is serialized in file
-            FW_ASSERT(Fw::FW_SERIALIZE_OK == buff.deserialize(recordSize));
+            desStat = buff.deserialize(recordSize);
+            FW_ASSERT(Fw::FW_SERIALIZE_OK == desStat);
 
             // sanity check value. It can't be larger than the maximum parameter buffer size + id
             // or smaller than the record id
@@ -338,7 +339,8 @@ namespace Svc {
             // reset deserialization
             buff.resetDeser();
             // deserialize, since parameter ID is serialized in file
-            FW_ASSERT(Fw::FW_SERIALIZE_OK == buff.deserialize(parameterId));
+            desStat = buff.deserialize(parameterId);
+            FW_ASSERT(Fw::FW_SERIALIZE_OK == desStat);
 
             // copy parameter
             this->m_db[entry].used = true;

diff --git a/config/FpConfig.h b/config/FpConfig.h
@@ -154,10 +154,12 @@ typedef U16 FwTlmPacketizeIdType;
 #endif
 
 // Set assertion form. Options:
-//   1. FW_NO_ASSERT: assertions are compiled out
+//   1. FW_NO_ASSERT: assertions are compiled out, side effects are kept
 //   2. FW_FILEID_ASSERT: asserts report a file CRC and line number
 //   3. FW_FILENAME_ASSERT: asserts report a file path (__FILE__) and line number
 //   4. FW_RELATIVE_PATH_ASSERT: asserts report a relative path within F´ or F´ library and line number
+//
+// Note: users who want alternate asserts should set assert level to FW_NO_ASSERT and define FW_ASSERT in this header
 #define FW_ASSERT_DFL_MSG_LEN 256  //!< Maximum assert message length when using the default assert handler
 #ifndef FW_ASSERT_LEVEL
 #define FW_ASSERT_LEVEL FW_FILENAME_ASSERT  //!< Defines the type of assert used