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

sys/sys: Add DEBUG_PANIC() macro #1471

Merged
merged 2 commits into from Oct 23, 2018

Conversation

ccollins476ad
Copy link
Contributor

The DEBUG_PANIC() macro triggers a crash if the DEBUG_PANIC_ENABLED syscfg setting is enabled. If this setting is disabled, the macro expands to nothing.

This feature is discussed briefly here: https://lists.apache.org/thread.html/0ce981b90eb74226fd660f25ba88f4ce6804eea2943edac70bf8731e@%3Cdev.mynewt.apache.org%3E

This macro just expands to a call to `__assert_func()`.  Unlike the
`assert()` macro, defining `NDEBUG` does not disable this macro.

This macro can be used to trigger an unconditional crash.
The `DEBUG_PANIC()` macro triggers a crash if the `DEBUG_PANIC_ENABLED`
syscfg setting is enabled.  If this setting is disabled, the macro
expands to nothing.
@sjanc
Copy link
Contributor

sjanc commented Oct 19, 2018

quick question, how this differ from assert?

@sjanc
Copy link
Contributor

sjanc commented Oct 19, 2018

nevermind, I see that those are just convenient macros around asserts

@ccollins476ad
Copy link
Contributor Author

ccollins476ad commented Oct 19, 2018

The dev list email probably goes into more detail than anyone is interested in :). The short answer is:

  • DEBUG_ASSERT() does not evaluate an expression to determine success / failure; it assumes failure. Since these failures are expected, I think it is likely that the application will always want to execute some code in the failure case, regardless of whether the macro is configured to trigger a crash. For example: logging a message to the console when the failure is detected.

  • DEBUG_ASSERT() is intended to be disabled in production builds, so it should always be followed by code that recovers from the failure. assert() is meant to ensure that failures that "should never happen" indeed never happen, so there is no need for recovery code. In addition, even though assert() can be disabled via NDEBUG, I am of the opinion that it should never be disabled, as it helps eliminate a lot of possibilities when debugging faults in the field.

Copy link
Contributor

@wes3 wes3 left a comment

Choose a reason for hiding this comment

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

Changes look good to me.

Copy link
Contributor

@mkiiskila mkiiskila left a comment

Choose a reason for hiding this comment

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

LGTM. Only additional thing I could think of is adding a 3rd option for DEBUG_PANIC(), which would call user defined routine (defined in app, most likely) which could e.g. log the error. But we can add that later if there's an actual use case for such a thing.

@ccollins476ad ccollins476ad merged commit decc256 into apache:master Oct 23, 2018
@ccollins476ad ccollins476ad deleted the debug_panic branch October 23, 2018 17:48
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.

None yet

4 participants