Memory alignment issues in TIME (32bit, MCP750, CCSDS_VER_2 config) #666
Comments
|
As of right now this is the final core code change for 6.8 - @jphickey |
|
When I test this I get the same results on extended vs. non-extended headers. I would not expect the setting of this config option to change the alignment requirements of packets as most packets do not contain a CFE_SB_MsgId_t directly. The reason there are just a few warnings in this build is because most of the alignment warnings were resolved in issue #437 and these are simply all that are left. |
Yes, that's why it's an issue we need to address now. Extended headers only fixes the alignment warning for packets that contain CFE_SB_MsgId_t. |
|
@jphickey could you clarify what's the hold-up on resolving this issue? Your comments above make it sound like we are not on the same page. |
The "CFE_SB_CmdHdr_t" and "CFE_SB_TlmHdr_t" types were not defined such that they would have compatible alignment with (and thereby allow safe casting to/from) a CFE_SB_Msg_t type. This changes the definition to be a union so that the types are aligned correctly.
Update the CFE_ES_ShellTlm_t, CFE_TIME_ToneSignalCmd_t, and CFE_TIME_FakeToneCmd_t to use the CFE_SB_TlmHdr_t/CFE_SB_CmdHdr_t types to define the buffer, rather than a uint8 array. This should not change the size, as it was already defined using sizeof() this structure, but it will make it aligned correctly which resolves the compiler warning. Note that all CMD/TLM should really be defined this way, but this only selectively changes the places that were actually generating a compiler warning about this. There is a risk that padding will be added, but this change should not change the padding or size of messages in 32-bit builds.
The challenge here was to come up with something that wouldn't just hide the fundamental flaw (i.e. address the actual alignment of the message structure at issue) but also wouldn't affect other messages. Hopefully the PR that I just submitted will be acceptable. The fix I'm proposing is split into two commits, the first just makes the The second commit changes the header definition only on these particular message types that were generating warnings. This makes them aligned and fixes the warning. Note the same thing should be done for all messages, because using a |
|
Also note this PR only fixes warnings within the FSW code. I'm still seeing several alignment cast warnings in the unit test code on my 32-bit test platform. Will submit a separate issue about that. |
jphickey
added
the
CCB:Ready
astrogeco
removed
the
CCB:Ready
|
Concurrence received on applying this pattern on all the messages, we'll manage the impact of tlm changes as part of this cycle. |
Fix #666, alignment of CMD/TLM message definitions
Update the CFE_ES_ShellTlm_t, CFE_TIME_ToneSignalCmd_t, and CFE_TIME_FakeToneCmd_t to use the CFE_SB_TlmHdr_t/CFE_SB_CmdHdr_t types to define the buffer, rather than a uint8 array. This should not change the size, as it was already defined using sizeof() this structure, but it will make it aligned correctly which resolves the compiler warning. Note that all CMD/TLM should really be defined this way, but this only selectively changes the places that were actually generating a compiler warning about this. There is a risk that padding will be added, but this change should not change the padding or size of messages in 32-bit builds.
Fix #666, alignment of CMD/TLM message definitions
Is your feature request related to a problem? Please describe.
Describe the solution you'd like
Force alignment where possible without changing bits on the wire
Describe alternatives you've considered
Remove
-Wcast-alignfor this buildAdditional context
Note there are other alignment issues for other configuration options (#313, #314) but they don't show up for MCP750 with CCSDS Version 2 so aren't critical to 6.8.
Requester Info
Jacob Hageman - NASA/GSFC
The text was updated successfully, but these errors were encountered: