-
Notifications
You must be signed in to change notification settings - Fork 222
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
Fix #688, implement value check and bug report macros #689
Conversation
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. |
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.
1bf057c
to
5d22ca6
Compare
This can be CCB reviewed in its current form, with the following caveats:
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.... |
CCB 2020-12-16 APPROVED
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 |
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 invokesBUGREPORT
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:
OSAL_CONFIG_BUGCHECK_DISABLE=false
andOSAL_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.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.OSAL_CONFIG_BUGCHECK_DISABLE=false
andOSAL_CONFIG_BUGCHECK_STRICT=true
. In this mode any BUGREPORT actions result in anabort()
which in turn intentionally causes the application to generate core dump if possible (i.e. ifulimit -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.