CFE_ES mempool returns buffers that are not aligned

[skliper]

Per email from Ryan Prentice to the cfs-community mailing list on 7/25:

Issues with Tables containing doubles

I tracked this down to be a MemPool issue. I’ve posted part of the stack trace below that is responsible for assigning memory from the mempool for the Table data.

CFE_ES_GetPoolBuf() - cfe_esmempool.c:389
CFE_TBL_Register() - cfe_tbl_api.c:295

Line 389 of cfe_esmempool.c sets the address of the pointer where the table data will be loaded.

{{{
*BufPtr = (uint32 *)(BdPtr + 1);
}}}

Below are some values I pulled out of the debugger.

{{{
BdPtr BD_t * 0x42A6D100
sizeof(*BdPtr) unsigned long int 0x0000000C
(uint32 *)(BdPtr + 1) uint32 * 0x42A6D10C
*BufPtr uint32 * 0x42A6D10C
}}}

In my table, the first piece of data is a double and the above code would have that double start at an address that is not double word aligned. To test that this is in fact that location that the problem originates, I added four bytes to *BufPtr and set it to 0x42A6D110 in the debugger, forcing it to be double word aligned. I then hit play and crossed my fingers, and what do ya know, it worked. Adding an int32 as the first member of the table allowed me to proceed past the issue, but I feel like that is a very fragile solution.

Issues with SB Messages containing doubles

I’m getting unaligned memory exceptions when receiving a message that contained doubles from the SB. It appears the Payload isn’t guaranteed to begin at a double word boundary. There is no way to pad for this since the Payload is aligned sometimes and unaligned others. I’ve pasted the stack trace below.

[skliper]

Imported from trac issue 199. Created by jphickey on 2017-07-26T15:12:46, last modified: 2019-03-05T14:58:28

[skliper]

Trac comment by jphickey on 2017-07-26 15:18:04:

From my perspective I had also noticed this issue a while back...

I have a patch that fixes it but it got lost in the shuffle because it became a moot point -- my projects were mostly 64 bit CPUs and/or did not have double types in telemetry so it never really showed up as an issue.

I found the patch and will push it up as a potential fix.

[skliper]

Trac comment by jphickey on 2017-07-27 10:38:59:

Commit [changeset:5bc5d96] has the proposed change for review.

This fixes additional issues beyond just the alignment of buffers.

[skliper]

Trac comment by jphickey on 2017-07-27 15:13:58:

After thinking about this a bit more I realized we can get the alignment requirement of any type by checking how much padding the compiler adds when putting it into a structure, i.e.

{{{
struct
{
char Byte;
AlignmentType_t Align;
};
}}}

The Align member would be padded out according to its alignment requirements. So essentially, the offset of this member should be equal to the alignment requirement.

By leveraging this relationship I was able to make a alternate patch that does not over-align (potentially wasting space) like the original one above. This also maintains more of the original behavior, i.e. errors out in the case of an unaligned buffer, rather than adjusting it.

This alternative option is in [changeset:f1e30f8]

[skliper]

Trac comment by glimes on 2017-08-01 13:32:42:

Additional note: once we have the memory allocator producing properly aligned blocks, we will have to follow up on the Message Buffer allocations. If messages are consistently aligned on 64-bit boundaries, Ryan's original problem will now go from "fails half the time" to "fails every time" ...

[skliper]

Trac comment by sstrege on 2017-08-04 13:06:14:

-----Original Message-----
From: Tashakkor, Scott B (MSFC-ES52)
Sent: Friday, August 04, 2017 9:02 AM
To: Strege, Susanne L. (GSFC-5820) [email protected]
Cc: Limes, Gregory L. (ARC-TI)[SGT, INC] [email protected]
Subject: Re: cFS CCB Meeting Minutes - 8-1-17

I approve of these changes.

Scott

[skliper]

Trac comment by jphickey on 2017-08-17 13:06:27:

Final proposal for this fix is ready, in commit [changeset:fd4d9da].

NOTE - This is all inclusive and rebased to development branch. I have deleted the other two proposals to avoid any confusion.

This is more of a departure from the original discussed changeset than what was intended. The reason is that the original proposal did not work as intended when using the new CFE_PLATFORM_ES_MEMPOOL_ALIGN_SIZE_MIN platform config.

As a result, I reverted some of the original change and now the buffer address calculation is done entirely in memory addresses.

I also noticed a potential bug in CFE_ES_GetPoolBufInfo where the buffer descriptor pointer was accessed after releasing the lock. I rearranged this code to avoid this, and also simplified the code paths (there is now only one return). A similar fix was done to CFE_ES_PutPoolBuf. In these functions there is now only one take and one give for the mutex, and anything that goes through the former must go through the latter.

[skliper]

Trac comment by sstrege on 2017-08-22 11:57:04:

Recommend adding a verification in es_verify.h to ensure CFE_PLATFORM_ES_MEMPOOL_ALIGN_SIZE_MIN is a power of 2.

[skliper]

Trac comment by sstrege on 2017-08-22 12:05:22:

cfe_es.h, line 345 – Fix comment to remove the word “most”.

[skliper]

Trac comment by jphickey on 2017-08-22 15:56:51:

Per CCB review on 2017-08-22 the follow-on commit [changeset:5c53b99] is ready for merge.

This commit corrects the comment on CFE_ES_PoolAlign_t and adds a verification that CFE_PLATFORM_ES_MEMPOOL_ALIGN_SIZE_MIN is a power of 2

The trac-199-fix-mempool-alignment branch has both commits and should be ready to merge into development.

Revised version also includes a whitespace fix in CFE_ES_PoolCreateEx

[skliper]

Trac comment by jphickey on 2017-08-23 12:06:55:

Noticed that issue #136 seems to be a duplicate of this -- was GSFC DCR 22813. Recommend closing #136 along with this one now that it is fixed.

[skliper]

Trac comment by jhageman on 2019-03-05 14:58:28:

Milestone renamed