Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid alignment warnings on some CPUs #450

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Jan 3, 2020

Describe the contribution

Partially address #313 (overlapping issue)

On CPUs with strict alignment requirements, some CFE code that uses a char-type pointer (e.g. uint8*) to compute memory addresses triggers an alignment warning when it gets cast back to the actual data type.

In the mempool implementation, the pointer should be sufficiently aligned already, because the address computation already takes CPU alignment requirements into account when calculating the addresses/offsets.

  • For the CFE_SB pool buffers, using the cpuaddr type, which is integer in nature, avoids the warning.
  • For the CFE_TBL internal table pointer, use a void* internally to store the buffer pointer, rather than a uint8_t*. This changes the casting needs elsewhere.

Testing performed
Build CFE with ENABLE_UNIT_TESTS=TRUE
Confirm all unit tests passing
Perform sanity test on CFE (normal startup, send commands from console)
Build for MIPS64 and ensure that (some) alignment warnings are fixed

Expected behavior changes
No change to behavior. Fixes build warnings only.

System(s) tested on:
Ubuntu 18.04 LTS 64-bit

Additional context
There are still some remaining alignment cast warnings regarding the message types, where a local message buffer is cast to a CFE_SB_Msg_t* which has a higher alignment requirement. This is a little harder to fix as it requires changing the local buffer definition.

Contributor Info
Joseph Hickey, Vantage Systems, Inc.

Community contributors
You must attach a signed CLA (required for acceptance) or reference one already submitted

On CPUs with strict alignment requirements, some CFE code
that uses a char-type pointer (e.g. uint8*) to compute
memory addresses triggers an alignment warning when it gets
cast back to the actual data type.

This code should be alignment-safe already, because the address
computation already takes CPU alignment requirements into account
when calculating the addresses/offsets.

However, the compiler still flags the final conversion from a
pointer with no special alignment to something with alignment
requirements.

- For the CFE_SB pool buffers, using the `cpuaddr` type, which
  is integer in nature, avoids the warning.
- For the CFE_TBL internal table pointer, use a `void*` internally
  to store the buffer pointer, rather than a `uint8_t*`.  This
  changes the casting needs elsewhere.
@jphickey
Copy link
Contributor Author

jphickey commented Jan 3, 2020

Update: changed the description to reflect that this will fix the char* casts cited in #437 but the older issue #313 also mentions issues related to the message types (CFE_SB_Msg_t* casts) that this does not fully address.

There will need to be an additional pull request to fix those ones.

@jphickey jphickey requested a review from CDKnightNASA January 3, 2020 15:26
Copy link
Contributor

@CDKnightNASA CDKnightNASA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, does this mean however that the pointer may not be aligned? I don't know if there are any CPU's that cannot interact with unaligned data but it will affect performance on many platforms. Should we add code to align?

@jphickey
Copy link
Contributor Author

jphickey commented Jan 7, 2020

does this mean however that the pointer may not be aligned? I don't know if there are any CPU's that cannot interact with unaligned data but it will affect performance on many platforms. Should we add code to align?

SPARC and MIPS processors (at least) will throw an exception when attempting to access a data structure that is not properly aligned. I believe ARM and x86 will auto-correct but there is a performance penalty to pay. Not sure about PowerPC.

For the items specifically in this pull request (the pool address calculation) the buffer descriptor is already a union including large types, such that sizeof(BD_t) will be a multiple of the system alignment required for the large. In turn all pool buffers are also already allocated to be a multiple of this size.

So all the pointers are already aligned sufficiently, the problem is merely that it was using uint8* intermediate pointer to perform the address calculations, and then casting a uint8* back to the intended type. This final step is what triggered a warning, just because the final type has a stricter alignment requirement than uint8* does.

By doing the intermediate address calculation as an integer (cpuaddr) the result is exactly the same but gcc doesn't warn about it.

@skliper skliper added this to the 6.8.0 milestone Jan 10, 2020
@skliper skliper added CCB:Approved Indicates code review and approval by community CCB enhancement labels Jan 10, 2020
@skliper
Copy link
Contributor

skliper commented Jan 10, 2020

CCB 20190108 - Reviewed and approved for integration candidate

@skliper skliper changed the base branch from master to merge-20200108 January 10, 2020 15:12
@skliper skliper merged commit 1ad6454 into nasa:merge-20200108 Jan 10, 2020
skliper added a commit that referenced this pull request Jan 10, 2020
Fix #259 #425 #427 #435 #437 #438 #443 #445
Reviewed and approved at 2020-01-08 CCB
skliper added a commit that referenced this pull request Jan 14, 2020
Fix #437: Avoid alignment warnings on some CPUs
@jphickey jphickey deleted the fix-437-alignment-warnings branch February 13, 2020 15:59
@skliper skliper linked an issue Sep 24, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alignment warnings when casting char* pointers
3 participants