Skip to content

Commit

Permalink
Fix #2240, improve 64-bit memory address handling in CMD/TLM
Browse files Browse the repository at this point in the history
The "CFE_ES_MemAddress_t" and "CFE_ES_MemOffset_t" types were intended
to provide a path for easily upgrading the CMD/TLM structs from 32-bit
to 64-bit memory addresses.  However, this type was a bit overused and
in some of those use-cases (e.g. in TBL header) it assumed that the type
was 32-bits during the byte swap ops.  As a result, the type could not
be changed to 64 bits as intended.

This reverts those cases in TBL back to uint32 (meaning that tables will
still be limited to 32 bit sizes, even on 64 bit CPUs) but otherwise the
addresses and sizes in ES/SB telemetry can grow to 64 bits as intended.

For unit tests, correct operation depends on the availablily of an updated
test macro that can compare integers as "size_t" type. (as opposed to
uint32).
  • Loading branch information
jphickey committed Mar 14, 2023
1 parent e35c3da commit 7fa0143
Show file tree
Hide file tree
Showing 19 changed files with 159 additions and 154 deletions.
15 changes: 0 additions & 15 deletions modules/cfe_assert/inc/cfe_assert.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,6 @@ typedef void (*CFE_Assert_StatusCallback_t)(uint8 MessageType, const char *Prefi
#define CFE_Assert_RESOURCEID_UNDEFINED(id) \
UtAssert_True(!CFE_RESOURCEID_TEST_DEFINED(id), "%s (0x%lx) not defined", #id, CFE_RESOURCEID_TO_ULONG(id))

/*****************************************************************************/
/**
** \brief Macro to check CFE memory size/offset for equality
**
** \par Description
** A macro that checks two memory offset/size values for equality.
**
** \par Assumptions, External Events, and Notes:
** This is a simple unsigned comparison which logs the values as hexadecimal
**
******************************************************************************/
#define CFE_Assert_MEMOFFSET_EQ(off1, off2) \
UtAssert_GenericUnsignedCompare(off1, UtAssert_Compare_EQ, off2, UtAssert_Radix_HEX, __FILE__, __LINE__, \
"Offset Check: ", #off1, #off2)

/*****************************************************************************/
/**
** \brief Macro to check CFE message ID for equality
Expand Down
46 changes: 18 additions & 28 deletions modules/cfe_testcase/src/es_info_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -70,24 +70,17 @@ void TestGetAppInfo(void)
TestAppInfo.FileName);
UtAssert_True(strlen(ESAppInfo.FileName) == 0, "ES App Info -> FileName = %s", ESAppInfo.FileName);

UtAssert_True(TestAppInfo.StackSize > 0, "Test App Info -> StackSz = %d", (int)TestAppInfo.StackSize);
UtAssert_True(ESAppInfo.StackSize > 0, "ES App Info -> StackSz = %d", (int)ESAppInfo.StackSize);
UtAssert_NONZERO(CFE_ES_MEMOFFSET_TO_SIZET(TestAppInfo.StackSize));
UtAssert_NONZERO(CFE_ES_MEMOFFSET_TO_SIZET(ESAppInfo.StackSize));

if (TestAppInfo.AddressesAreValid)
{
UtAssert_True(TestAppInfo.AddressesAreValid > 0, "Test App Info -> AddrsValid? = %d",
(int)TestAppInfo.AddressesAreValid);
UtAssert_True(TestAppInfo.CodeAddress > 0, "Test App Info -> CodeAddress = %ld",
(unsigned long)TestAppInfo.CodeAddress);
UtAssert_True(TestAppInfo.CodeSize > 0, "Test App Info -> CodeSize = %ld",
(unsigned long)TestAppInfo.CodeSize);
UtAssert_True(TestAppInfo.DataAddress > 0, "Test App Info -> DataAddress = %ld",
(unsigned long)TestAppInfo.DataAddress);
UtAssert_True(TestAppInfo.DataSize > 0, "Test App Info -> DataSize = %ld",
(unsigned long)TestAppInfo.DataSize);
UtAssert_True(TestAppInfo.BSSAddress > 0, "Test App Info -> BSSAddress = %ld",
(unsigned long)TestAppInfo.BSSAddress);
UtAssert_True(TestAppInfo.BSSSize > 0, "Test App Info -> BSSSize = %ld", (unsigned long)TestAppInfo.BSSSize);
UtAssert_NOT_NULL(CFE_ES_MEMADDRESS_TO_PTR(TestAppInfo.CodeAddress));
UtAssert_NONZERO(CFE_ES_MEMOFFSET_TO_SIZET(TestAppInfo.CodeSize));
UtAssert_NOT_NULL(CFE_ES_MEMADDRESS_TO_PTR(TestAppInfo.DataAddress));
UtAssert_NONZERO(CFE_ES_MEMOFFSET_TO_SIZET(TestAppInfo.DataSize));
UtAssert_NOT_NULL(CFE_ES_MEMADDRESS_TO_PTR(TestAppInfo.BSSAddress));
UtAssert_NONZERO(CFE_ES_MEMOFFSET_TO_SIZET(TestAppInfo.BSSSize));
}
else
{
Expand All @@ -102,10 +95,8 @@ void TestGetAppInfo(void)
UtAssert_True(ESAppInfo.AddressesAreValid == 0, "ES App Info -> AddrsValid? = %d",
(int)ESAppInfo.AddressesAreValid);

UtAssert_True(TestAppInfo.StartAddress > 0, "Test App Info -> StartAddress = 0x%8lx",
(unsigned long)TestAppInfo.StartAddress);
UtAssert_True(ESAppInfo.StartAddress == 0, "ES App Info -> StartAddress = 0x%8lx",
(unsigned long)ESAppInfo.StartAddress);
UtAssert_NOT_NULL(CFE_ES_MEMADDRESS_TO_PTR(TestAppInfo.StartAddress));
UtAssert_NULL(CFE_ES_MEMADDRESS_TO_PTR(ESAppInfo.StartAddress));

UtAssert_INT32_EQ(TestAppInfo.ExceptionAction, 0);
UtAssert_INT32_EQ(ESAppInfo.ExceptionAction, 1);
Expand Down Expand Up @@ -180,18 +171,17 @@ void TestGetLibInfo(void)
UtAssert_StrCmp(LibInfo.EntryPoint, "CFE_Assert_LibInit", "Lib Info -> EntryPt = %s", LibInfo.EntryPoint);
UtAssert_True(strstr(LibInfo.FileName, FileName) != NULL, "Lib Info -> FileName = %s contains %s", LibInfo.FileName,
FileName);
UtAssert_True(LibInfo.StackSize == 0, "Lib Info -> StackSz = %d", (int)LibInfo.StackSize);

UtAssert_ZERO(CFE_ES_MEMOFFSET_TO_SIZET(LibInfo.StackSize));

if (LibInfo.AddressesAreValid)
{
UtAssert_True(LibInfo.AddressesAreValid > 0, "Lib Info -> AddrsValid? = %ld",
(unsigned long)LibInfo.AddressesAreValid);
UtAssert_True(LibInfo.CodeAddress > 0, "Lib Info -> CodeAddress = %ld", (unsigned long)LibInfo.CodeAddress);
UtAssert_True(LibInfo.CodeSize > 0, "Lib Info -> CodeSize = %ld", (unsigned long)LibInfo.CodeSize);
UtAssert_True(LibInfo.DataAddress > 0, "Lib Info -> DataAddress = %ld", (unsigned long)LibInfo.DataAddress);
UtAssert_True(LibInfo.DataSize > 0, "Lib Info -> DataSize = %ld", (unsigned long)LibInfo.DataSize);
UtAssert_True(LibInfo.BSSAddress > 0, "Lib Info -> BSSAddress = %ld", (unsigned long)LibInfo.BSSAddress);
UtAssert_True(LibInfo.BSSSize > 0, "Lib Info -> BSSSize = %ld", (unsigned long)LibInfo.BSSSize);
UtAssert_NOT_NULL(CFE_ES_MEMADDRESS_TO_PTR(LibInfo.CodeAddress));
UtAssert_NONZERO(CFE_ES_MEMOFFSET_TO_SIZET(LibInfo.CodeSize));
UtAssert_NOT_NULL(CFE_ES_MEMADDRESS_TO_PTR(LibInfo.DataAddress));
UtAssert_NONZERO(CFE_ES_MEMOFFSET_TO_SIZET(LibInfo.DataSize));
UtAssert_NOT_NULL(CFE_ES_MEMADDRESS_TO_PTR(LibInfo.BSSAddress));
UtAssert_NONZERO(CFE_ES_MEMOFFSET_TO_SIZET(LibInfo.BSSSize));
}
else
{
Expand Down
4 changes: 2 additions & 2 deletions modules/cfe_testcase/src/es_mempool_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,10 @@ void TestMemPoolDelete(void)
UtAssert_INT32_EQ(CFE_ES_PoolCreateEx(&PoolID, Buffer, sizeof(Buffer), 0, NULL, CFE_ES_NO_MUTEX), CFE_SUCCESS);
UtAssert_INT32_EQ(CFE_ES_GetMemPoolStats(&Stats, PoolID), CFE_SUCCESS);

UtAssert_UINT32_EQ(Stats.PoolSize, sizeof(Buffer));
UtAssert_EQ(size_t, CFE_ES_MEMOFFSET_TO_SIZET(Stats.PoolSize), sizeof(Buffer));
UtAssert_UINT32_EQ(Stats.NumBlocksRequested, 0);
UtAssert_UINT32_EQ(Stats.CheckErrCtr, 0);
UtAssert_UINT32_EQ(Stats.NumFreeBytes, sizeof(Buffer));
UtAssert_EQ(size_t, CFE_ES_MEMOFFSET_TO_SIZET(Stats.NumFreeBytes), sizeof(Buffer));

UtAssert_INT32_EQ(CFE_ES_GetMemPoolStats(NULL, PoolID), CFE_ES_BAD_ARGUMENT);
UtAssert_INT32_EQ(CFE_ES_GetMemPoolStats(&Stats, CFE_ES_MEMHANDLE_UNDEFINED), CFE_ES_ERR_RESOURCEID_NOT_VALID);
Expand Down
23 changes: 12 additions & 11 deletions modules/cfe_testcase/src/tbl_content_mang_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -248,22 +248,23 @@ void TestModified(void)
UtAssert_INT32_EQ(CFE_TBL_Modified(CFE_TBL_BAD_TABLE_HANDLE), CFE_TBL_ERR_INVALID_HANDLE);
}

/* Helper function to set a CFE_ES_MemOffset_t value (must be big-endian) */
void TblTest_UpdateOffset(CFE_ES_MemOffset_t *TgtVal, CFE_ES_MemOffset_t SetVal)
/* Helper function to set a 32-bit table offset value (must be big-endian) */
void TblTest_UpdateOffset(uint32 *TgtVal, size_t SetVal)
{
size_t i;
union
{
CFE_ES_MemOffset_t offset;
uint8 bytes[sizeof(CFE_ES_MemOffset_t)];
uint32 offset;
uint8 bytes[sizeof(uint32)];
} offsetbuf;

offsetbuf.bytes[3] = SetVal & 0xFF;
SetVal >>= 8;
offsetbuf.bytes[2] = SetVal & 0xFF;
SetVal >>= 8;
offsetbuf.bytes[1] = SetVal & 0xFF;
SetVal >>= 8;
offsetbuf.bytes[0] = SetVal & 0xFF;
i = sizeof(offsetbuf.bytes);
while (i > 0)
{
--i;
offsetbuf.bytes[i] = SetVal & 0xFF;
SetVal >>= 8;
}

*TgtVal = offsetbuf.offset;
}
Expand Down
28 changes: 28 additions & 0 deletions modules/core_api/eds/base_types.xml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,34 @@
<StringDataType name="ApiName" length="${CFE_MISSION/MAX_API_LEN}" />
<StringDataType name="PathName" length="${CFE_MISSION/MAX_PATH_LEN}" />

<!--
Memory addresses in CMD/TLM: These are integer types based on the
CFE_MISSION/MEM_ADDR_SIZE_BITS configuration setting. This allows
the user to select 32-bit (traditional) or 64-bit (modern) integer
values to be used in CMD/TLM fields that store a memory address.
Note that changing from 32 to 64 will extend all containers that
use/reference this type by a proportional amount, so traditional
non-EDS 32-bit CMD/TLM definitions will NOT match when this is 64 bits.
-->
<IntegerDataType name="MemReference" shortDescription="Integer type used for CPU memory addresses, sizes and offsets">
<LongDescription>
For backward compatibility with existing CFS code this should be uint32,
but all telemetry information will be limited to 4GB in size as a result.

On 64-bit platforms this can be expanded to 64 bits which will allow larger
memory objects, but this will break compatibility with existing control
systems, and may also change the alignment/padding of some messages.

In either case this must be an unsigned type, and should be large enough
to represent the largest memory address/size in use in the CFS system.
</LongDescription>
<IntegerDataEncoding sizeInBits="${CFE_MISSION/MEM_REFERENCE_SIZE_BITS}" encoding="unsigned" />
<Range>
<MinMaxRange max="2 ^ ${CFE_MISSION/MEM_REFERENCE_SIZE_BITS}" min="0" rangeType="inclusiveMinExclusiveMax"/>
</Range>
</IntegerDataType>

</DataTypeSet>

</Package>
Expand Down
30 changes: 23 additions & 7 deletions modules/core_api/fsw/inc/cfe_es_extern_typedefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,21 @@ typedef uint16 CFE_ES_TaskPriority_Atom_t;
*/
typedef uint32 CFE_ES_MemOffset_t;

/*
/**
* @brief Memory Offset initializer wrapper
*
* A converter macro to use when initializing a CFE_ES_MemOffset_t
* from an integer value of a different type.
*/
#define CFE_ES_MEMOFFSET_C(x) ((CFE_ES_MemOffset_t)(x))
#define CFE_ES_MEMOFFSET_C(x) ((CFE_ES_MemOffset_t)(x))

/**
* @brief Memory Offset to integer value (size_t) wrapper
*
* A converter macro to use when interpreting a CFE_ES_MemOffset_t
* value as a "size_t" type
*/
#define CFE_ES_MEMOFFSET_TO_SIZET(x) ((size_t)(x))

/**
* @brief Type used for memory addresses in command and telemetry messages
Expand All @@ -408,15 +418,21 @@ typedef uint32 CFE_ES_MemOffset_t;
*/
typedef uint32 CFE_ES_MemAddress_t;

/*
/**
* @brief Memory Address initializer wrapper
*
* A converter macro to use when initializing a CFE_ES_MemAddress_t
* from a pointer value of a different type.
*/
#define CFE_ES_MEMADDRESS_C(x) ((CFE_ES_MemAddress_t)((cpuaddr)(x)&0xFFFFFFFF))

/**
* @brief Memory Address to pointer wrapper
*
* @note on a 64 bit platform, this macro will truncate the address such
* that it will fit into a 32-bit telemetry field. Obviously, the resulting
* value is no longer usable as a memory address after this.
* A converter macro to use when interpreting a CFE_ES_MemAddress_t
* as a pointer value.
*/
#define CFE_ES_MEMADDRESS_C(x) ((CFE_ES_MemAddress_t)((cpuaddr)(x)&0xFFFFFFFF))
#define CFE_ES_MEMADDRESS_TO_PTR(x) ((void *)(cpuaddr)(x))

/*
* Data Structures shared between API and Message (CMD/TLM) interfaces
Expand Down
12 changes: 8 additions & 4 deletions modules/core_api/fsw/inc/cfe_tbl_extern_typedefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,17 @@ typedef uint16 CFE_TBL_BufferSelect_Enum_t;
* @brief The definition of the header fields that are included in CFE Table Data files.
*
* This header follows the CFE_FS header and precedes the actual table data.
*
* @note The Offset and NumBytes fields in the table header are to 32 bits for
* backward compatibility with existing CFE versions. This means that even on
* 64-bit CPUs, individual table files will be limited to 4GiB in size.
*/
typedef struct CFE_TBL_File_Hdr
{
uint32 Reserved; /**< Future Use: NumTblSegments in File? */
CFE_ES_MemOffset_t Offset; /**< Byte Offset at which load should commence */
CFE_ES_MemOffset_t NumBytes; /**< Number of bytes to load into table */
char TableName[CFE_MISSION_TBL_MAX_FULL_NAME_LEN]; /**< Fully qualified name of table to load */
uint32 Reserved; /**< Future Use: NumTblSegments in File? */
uint32 Offset; /**< Byte Offset at which load should commence */
uint32 NumBytes; /**< Number of bytes to load into table */
char TableName[CFE_MISSION_TBL_MAX_FULL_NAME_LEN]; /**< Fully qualified name of table to load */
} CFE_TBL_File_Hdr_t;

#endif /* CFE_EDS_ENABLED_BUILD */
Expand Down
15 changes: 0 additions & 15 deletions modules/core_private/ut-stubs/inc/ut_support.h
Original file line number Diff line number Diff line change
Expand Up @@ -753,21 +753,6 @@ bool CFE_UtAssert_MessageCheck_Impl(bool Status, const char *File, uint32 Line,
UtAssert_GenericUnsignedCompare(CFE_RESOURCEID_TO_ULONG(id1), UtAssert_Compare_EQ, CFE_RESOURCEID_TO_ULONG(id2), \
UtAssert_Radix_HEX, __FILE__, __LINE__, "Resource ID Check: ", #id1, #id2)

/*****************************************************************************/
/**
** \brief Macro to check CFE memory size/offset for equality
**
** \par Description
** A macro that checks two memory offset/size values for equality.
**
** \par Assumptions, External Events, and Notes:
** This is a simple unsigned comparison which logs the values as hexadecimal
**
******************************************************************************/
#define CFE_UtAssert_MEMOFFSET_EQ(off1, off2) \
UtAssert_GenericUnsignedCompare(off1, UtAssert_Compare_EQ, off2, UtAssert_Radix_HEX, __FILE__, __LINE__, \
"Offset Check: ", #off1, #off2)

/*****************************************************************************/
/**
** \brief Macro to check CFE message ID for equality
Expand Down
38 changes: 18 additions & 20 deletions modules/es/eds/cfe_es.xml
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@
</Range>
</IntegerDataType>

<IntegerDataType name="MemOffset" shortDescription="Type used for memory sizes and offsets in commands and telemetry">
<ContainerDataType name="MemOffset" shortDescription="Type used for memory sizes and offsets in commands and telemetry">
<LongDescription>
For backward compatibility with existing CFE code this should be uint32,
but all telemetry information will be limited to 4GB in size as a result.
Expand All @@ -176,13 +176,12 @@

In either case this must be an unsigned type.
</LongDescription>
<IntegerDataEncoding sizeInBits="${CFE_ES/MEM_OFFSET_SIZE_BITS}" encoding="unsigned" />
<Range>
<MinMaxRange max="2 ^ ${CFE_ES/MEM_OFFSET_SIZE_BITS}" min="0" rangeType="inclusiveMinExclusiveMax"/>
</Range>
</IntegerDataType>
<EntryList>
<Entry name="Offset" type="BASE_TYPES/MemReference" shortDescription="Memory Offset" />
</EntryList>
</ContainerDataType>

<IntegerDataType name="MemAddress" shortDescription="Type used for memory addresses in command and telemetry messages">
<ContainerDataType name="MemAddress" shortDescription="Type used for memory addresses in command and telemetry messages">
<LongDescription>
For backward compatibility with existing CFE code this should be uint32,
but if running on a 64-bit platform, addresses in telemetry will be
Expand All @@ -200,11 +199,10 @@
provides independence between the message representation and local
representation of a memory address.
</LongDescription>
<IntegerDataEncoding sizeInBits="${CFE_ES/MEM_OFFSET_SIZE_BITS}" encoding="unsigned" />
<Range>
<MinMaxRange max="2 ^ ${CFE_ES/MEM_OFFSET_SIZE_BITS}" min="0" rangeType="inclusiveMinExclusiveMax"/>
</Range>
</IntegerDataType>
<EntryList>
<Entry name="Addr" type="BASE_TYPES/MemReference" shortDescription="Memory Address" />
</EntryList>
</ContainerDataType>

<StringDataType name="char_x_CFE_ES_CDS_MAX_FULL_NAME_LEN" length="${CFE_MISSION/ES_CDS_MAX_FULL_NAME_LEN}" />

Expand Down Expand Up @@ -246,7 +244,7 @@
\cfetlmmnemonic \ES_APPFILENAME
</LongDescription>
</Entry>
<Entry name="StackSize" type="BASE_TYPES/uint32" shortDescription="The Stack Size of the Application">
<Entry name="StackSize" type="MemOffset" shortDescription="The Stack Size of the Application">
<LongDescription>
\cfetlmmnemonic \ES_STACKSIZE
</LongDescription>
Expand Down Expand Up @@ -353,7 +351,7 @@

<ContainerDataType name="BlockStats" shortDescription="Memory Pool Statistics data type">
<EntryList>
<Entry name="BlockSize" type="BASE_TYPES/uint32" shortDescription="Number of bytes in each of these blocks" />
<Entry name="BlockSize" type="MemOffset" shortDescription="Number of bytes in each of these blocks" />
<Entry name="NumCreated" type="BASE_TYPES/uint32" shortDescription="Number of Memory Blocks of this size created" />
<Entry name="NumFree" type="BASE_TYPES/uint32" shortDescription="Number of Memory Blocks of this size that are free" />
</EntryList>
Expand Down Expand Up @@ -432,7 +430,7 @@
<Entry name="Application" type="BASE_TYPES/ApiName" shortDescription="Name of Application to be started" />
<Entry name="AppEntryPoint" type="BASE_TYPES/ApiName" shortDescription="Symbolic name of Application's entry point" />
<Entry name="AppFileName" type="BASE_TYPES/PathName" shortDescription="Full path and filename of Application's executable image" />
<Entry name="StackSize" type="BASE_TYPES/uint32" shortDescription="Desired stack size for the new application" />
<Entry name="StackSize" type="MemOffset" shortDescription="Desired stack size for the new application" />
<Entry name="ExceptionAction" type="ExceptionAction">
<LongDescription>
\brief #CFE_ES_ExceptionAction_RESTART_APP=On exception, restart Application,
Expand Down Expand Up @@ -632,12 +630,12 @@
\cfetlmmnemonic \ES_PSPMISSIONREV
</LongDescription>
</Entry>
<Entry name="SysLogBytesUsed" type="BASE_TYPES/uint32" shortDescription="Total number of bytes used in system log">
<Entry name="SysLogBytesUsed" type="MemOffset" shortDescription="Total number of bytes used in system log">
<LongDescription>
\cfetlmmnemonic \ES_SYSLOGBYTEUSED
</LongDescription>
</Entry>
<Entry name="SysLogSize" type="BASE_TYPES/uint32" shortDescription="Total size of the system log">
<Entry name="SysLogSize" type="MemOffset" shortDescription="Total size of the system log">
<LongDescription>
\cfetlmmnemonic \ES_SYSLOGSIZE
</LongDescription>
Expand Down Expand Up @@ -752,17 +750,17 @@
\cfetlmmnemonic \ES_PERFDATA2WRITE
</LongDescription>
</Entry>
<Entry name="HeapBytesFree" type="BASE_TYPES/uint32" shortDescription="Number of free bytes remaining in the OS heap">
<Entry name="HeapBytesFree" type="MemOffset" shortDescription="Number of free bytes remaining in the OS heap">
<LongDescription>
\cfetlmmnemonic \ES_HEAPBYTESFREE
</LongDescription>
</Entry>
<Entry name="HeapBlocksFree" type="BASE_TYPES/uint32" shortDescription="Number of free blocks remaining in the OS heap">
<Entry name="HeapBlocksFree" type="MemOffset" shortDescription="Number of free blocks remaining in the OS heap">
<LongDescription>
\cfetlmmnemonic \ES_HEAPBLKSFREE
</LongDescription>
</Entry>
<Entry name="HeapMaxBlockSize" type="BASE_TYPES/uint32" shortDescription="Number of bytes in the largest free block">
<Entry name="HeapMaxBlockSize" type="MemOffset" shortDescription="Number of bytes in the largest free block">
<LongDescription>
\cfetlmmnemonic \ES_HEAPMAXBLK
</LongDescription>
Expand Down
Loading

0 comments on commit 7fa0143

Please sign in to comment.