search

nasa / cFE

The Core Flight System (cFS) Core Flight Executive (cFE)
Apache License 2.0
408 stars 200 forks source link

Memory alignment issues in TIME (32bit, MCP750, CCSDS_VER_2 config) #666

Closed skliper closed 4 years ago

skliper commented 4 years ago

Is your feature request related to a problem? Please describe.

/home/jhageman/cFS/cFS-GitHub/cfe/fsw/cfe-core/src/time/cfe_time_tone.c: In function 'CFE_TIME_Tone1HzTask':
/home/jhageman/cFS/cFS-GitHub/cfe/fsw/cfe-core/src/time/cfe_time_tone.c:1220: warning: cast increases required alignment of target type
/home/jhageman/cFS/cFS-GitHub/cfe/fsw/cfe-core/src/time/cfe_time_tone.c:1228: warning: cast increases required alignment of target type
/home/jhageman/cFS/cFS-GitHub/cfe/fsw/cfe-core/src/time/cfe_time_tone.c: In function 'CFE_TIME_Local1HzTask':
/home/jhageman/cFS/cFS-GitHub/cfe/fsw/cfe-core/src/time/cfe_time_tone.c:1437: warning: cast increases required alignment of target type

Describe the solution you'd like Force alignment where possible without changing bits on the wire

Describe alternatives you've considered Remove -Wcast-align for this build

Additional 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

skliper commented 4 years ago

As of right now this is the final core code change for 6.8 - @jphickey

jphickey commented 4 years ago

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.

skliper commented 4 years ago

When I test this I get the same results on extended vs. non-extended headers.

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.

skliper commented 4 years ago

@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.

jphickey commented 4 years ago

@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 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 CFE_SB_CmdHdr_t/CFE_SB_TlmHdr_t types correctly aligned for how they are actually used. Note most message definitions do not use this type - they define the message header as a uint8 array based on the sizeof() this type (i.e. not aligned).

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 uint8[] array is inherently unsafe/incorrect as it is not aligned for anything. It is only because most payloads contain a uint32 somewhere that it isn't getting warned about elsewhere.

jphickey commented 4 years ago

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.

skliper commented 4 years ago

Concurrence received on applying this pattern on all the messages, we'll manage the impact of tlm changes as part of this cycle.