From 1caf1e4be8246aa5fe3a44b66cf5ebf41be5f4c0 Mon Sep 17 00:00:00 2001 From: M Starch Date: Thu, 4 Jan 2024 17:02:13 -0800 Subject: [PATCH] Correcting NO_ASSERT handling and safety. Fixes #1706, #2447 (#2448) --- Fw/Types/Assert.cpp | 7 --- Fw/Types/Assert.hpp | 16 +++--- Fw/Types/test/ut/TypesTest.cpp | 35 +++++++++++++ Os/Posix/IntervalTimer.cpp | 4 +- Ref/SendBuffApp/SendBuffComponentImpl.cpp | 9 ++-- .../test/ut/AssertFatalAdapterTester.cpp | 51 +++++++++++++------ Svc/GenericHub/GenericHubComponentImpl.cpp | 23 ++++++--- Svc/PrmDb/PrmDbImpl.cpp | 6 ++- config/FpConfig.h | 4 +- 9 files changed, 112 insertions(+), 43 deletions(-) diff --git a/Fw/Types/Assert.cpp b/Fw/Types/Assert.cpp index 4a9def471e..be7a0a84ed 100644 --- 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 index a78819a691..5383c774e5 100644 --- a/Fw/Types/Assert.hpp +++ b/Fw/Types/Assert.hpp @@ -3,15 +3,20 @@ #include -#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 index 9cdc543635..7f329a3af9 100644 --- a/Fw/Types/test/ut/TypesTest.cpp +++ b/Fw/Types/test/ut/TypesTest.cpp @@ -847,36 +847,57 @@ 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()); @@ -884,9 +905,14 @@ void AssertTest() { 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 index 73c9a35c73..0ba7245883 100644 --- 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 index 8d8865a6dc..d8f5cb789e 100644 --- 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 index 91a076af42..d02f5000eb 100644 --- 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 index 1186378f72..fd606c259c 100644 --- 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 index 540ce5b162..a4cceb7e06 100644 --- 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 index 12e92bd7d6..0ffa821fb9 100644 --- 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