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

Use appropriate atomic type for inter-thread sync #1034

Closed
skliper opened this issue Sep 30, 2019 · 5 comments · Fixed by #1035 or #1047
Closed

Use appropriate atomic type for inter-thread sync #1034

skliper opened this issue Sep 30, 2019 · 5 comments · Fixed by #1035 or #1047
Labels
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

The RV tool analysis reported several cases of reading/writing shared memory variables without a lock.

The intention behind the code was that the data type being read/written simultaneously here was atomic in nature, thus the parallel access would be safe, as it is not possible to catch an atomic value mid-update.

To be more portable the code should use the C99 type sigatomic_t to ensure that the data type is in fact atomic on the given platform. Currently it is using uint32 which is not guaranteed to be atomic on all platforms.

Calling this "minor" because the uint32 type will be atomic on all the platforms that the code in question is actually used on. There is no bug currently here, this is just to prevent a future bug if this code is expanded to smaller CPUs (microcontrollers) where uint32 is not atomic.

@skliper skliper self-assigned this Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 183. Created by jphickey on 2017-02-14T13:00:24, last modified: 2019-08-14T14:11:46

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2017-02-14 13:43:06:

Also need to make sure that we include the volatile keyword on these globals as well.

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by glimes on 2017-02-14 13:47:43:

The case we hit in Biosentinel is in CFE, I'll go update the
ticket over there -- but it's due to CFE_ES_ApplicationSyncDelay
spinning on CFE_ES_Global.SystemState and .AppReadyCount, which
are not themselves marked as volatile.

@skliper skliper assigned jphickey and unassigned skliper Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Dec 3, 2020

Issue described (using uint32 for CFE_ES_Global.SystemState) still exists as far as I can tell, but that's a cFE problem not applicable to any officially supported systems (only a system where a write to uint32 isn't atomic). Couldn't find a related cFE ticket other than #17, which doesn't directly address this issue. Moving to cFE.

@skliper skliper transferred this issue from nasa/osal Dec 3, 2020
@skliper skliper added the bug label Dec 3, 2020
@skliper
Copy link
Contributor Author

skliper commented Dec 3, 2020

Interesting inconsistencies:

/**
* @brief The overall cFE System State
*
*
* These values are used with the #CFE_ES_WaitForSystemState API call to synchronize application startup.
*
* @note These are defined in order so that relational comparisons e.g. if (STATEA < STATEB) are possible
*
* @sa enum CFE_ES_SystemState
*/
typedef uint32 CFE_ES_SystemState_Enum_t;

yet:
CFE_Status_t CFE_ES_WaitForSystemState(uint32 MinSystemState, uint32 TimeOutMilliseconds);

Note unique either (CFE_ES_RunStatus_Enum_t defined but unused). Seems like then intension was to use these types in the APIs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants