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

Fix #688, implement value check and bug report macros #689

Merged
merged 2 commits into from
Dec 18, 2020

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Dec 14, 2020

Describe the contribution
Provide a set of macros to facilitate the argument value checking typically performed by every public API.

  • BUGREPORT() is a printf-like macro that reports an invalid/unexpected condition has been found which indicates a bug in the application (i.e. uncorrectable).
  • BUGCHECK() provides a simplified method to confirm a condition is true. If not true, then it invokes BUGREPORT and (possibly) returns an error code to the caller. This is used for items which must be true or else indicate serious bugs where execution cannot/should not continue normally.
  • ARGCHECK() confirms a condition is true, but amy take a mitigation/corrective action rather than treat it as a bug if false. This can be used for "normal" range checking which should always produce a soft failure/error code response or enforce a suitable minimum/maximum value.

Fixes #688

Testing performed
Execute the "bin sem" tests - Note only the "bin sem" implementation has been updated to use these macros thus far.
Confirm the basic modes work:

  1. Normal/default mode where OSAL_CONFIG_BUGCHECK_DISABLE=false and OSAL_CONFIG_BUGCHECK_STRICT=false. In this case the error code is returned and the test passes normally. No change to behavior, but new error printfs are visible when running the test programs and they pass in bad values. Note: this is the historical behavior and the application keeps running. This requires the application to actually check/handle the error code. As these indicate bugs, It is quite likely that the application does not expect the error response and/or is already in a bad state such that it will not handle it correctly, causing a more obfuscated failure later on.
  2. Efficient mode where OSAL_CONFIG_BUGCHECK_DISABLE=true (OSAL_CONFIG_BUGCHECK_STRICT is not used). May be used by applications that have reached a high level of stability and have been confirmed never to invoke functions with outrightly bad arguments. Bug checks are not performed at all, but other argument checks still are.
  3. Strict mode where OSAL_CONFIG_BUGCHECK_DISABLE=false and OSAL_CONFIG_BUGCHECK_STRICT=true. In this mode any BUGREPORT actions result in an abort() which in turn intentionally causes the application to generate core dump if possible (i.e. if ulimit -c permits). The core file may then be opened via debugger to determine the cause of the failure with full context available. The intent here is to catch the error early while the context/stack is still present, and avoid the obfuscation that is likely to occur with (1).

Expected behavior changes
No impact to behavior (beyond additional debug printfs) when using default mode.
Other modes change behavior as noted above.

System(s) tested on
Ubuntu 20.04

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey
Copy link
Contributor Author

FOR REVIEW: only commit 2f83626 is relevant. Will rebase with next baseline.

Looking for feedback as to whether this is the desired approach before updating other code to use the macros more extensively. Currently only bin sem is done.

@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Dec 14, 2020
Add macros for configurable behavior of argument bug checking.

- BUGCHECK for checking argument values which should never happen
   and indicate bugs if they do.
- ARGCHECK for checking argument values which may happen and can
   be mitigated if they do.

The behavior of BUGCHECK is influenced by two new OSAL config
options, which can disable it completely or make it strict/enforcing
such that it will abort() for debugging if a condition is not met.
Use the new macros to validate arguments to OS API calls.

Also includes coverage test updates, which revealed some
areas where return types were not consistent, and the macro
makes them consistent now.
@jphickey jphickey changed the base branch from main to integration-candidate December 15, 2020 17:00
@jphickey
Copy link
Contributor Author

This can be CCB reviewed in its current form, with the following caveats:

  1. It is based on an intermediate integration-candidate -- pending rebase to main whenever the next merge is finalized.
  2. Unit test depends on the error checking in its current form (returning the expected error code). All unit tests pass with default check settings, however if the check macros are customized/changed (either disabled or strict or some other mod) then much work is still needed in order to make the tests skip all the cases which depend on this error checking.

Fixing up the unit tests to support alternate arg check configs could take a bit of time (hundreds of test cases) so it may be worth doing as a separate PR....

@jphickey jphickey linked an issue Dec 15, 2020 that may be closed by this pull request
@jphickey jphickey marked this pull request as ready for review December 15, 2020 17:16
@astrogeco astrogeco added CCB-20201216 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Dec 16, 2020
@astrogeco
Copy link
Contributor

astrogeco commented Dec 16, 2020

CCB 2020-12-16 APPROVED

  • Replaces one-off argument checks scattered across OSAL and cFE
  • Adds new items for debugging to default_config cmake file.

There are some situations where arguments aren't checked or could be optimized. There are some open issues for null checks.

JH EDIT - added link to unit-test issue

@astrogeco
Copy link
Contributor

astrogeco commented Dec 18, 2020

@jphickey did you create issue to fix unit-test?

JH EDIT - See #700

@astrogeco astrogeco merged commit d062b64 into nasa:integration-candidate Dec 18, 2020
astrogeco added a commit to nasa/cFS that referenced this pull request Dec 18, 2020
@jphickey jphickey deleted the fix-688-bugcheck-macro branch December 18, 2020 17:56
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement macro to facilitate argument checking
3 participants