You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by David Sidrane <da...@nscdg.com> on 2022/02/14 17:56:03 UTC
[DISCUSS] Default state of NDEBUG
PR 5399 adds an Kconfig option for NDEBUG. The salient discussion begins at
[2] there are mixed positions and reasoning. xiaoxiang781216 asked me to
raise a discussion on this.
The reasoning for the Default state of to be NDEBUG (n) hence undefined so
that assert() enabled is the following:
1) It follows the standard understanding of NDEBUG
The standard for standard library from [3]
The definition of the macro assert depends on another macro,
NDEBUG, *which is not defined* by the standard library.
2) We have DEBUGASSERT for use in the OS. I believe this was an
intentional separation on Greg’s part. We have asked for is input.
In a NuttX "Release" build DEGUASSERT is off (all debug is off to show off
the build size).
I should still be able to build the app code with assert() and not have to
use a Kconfig to enable it.
How would you prefer it to be defined?
1. Defaulted ON – assert() is a No OP
2. Defaulted OFF assert() is enabled.
3. Left to a command line setting from build system
David
[1] https://github.com/apache/incubator-nuttx/pull/5399
[2]
https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029387606
[3]
https://en.cppreference.com/w/c/error/assert#:~:text=If%20NDEBUG%20is%20defined%20as,the%20source%20code%20where%20%3Cassert.&text=If%20NDEBUG%20is%20not%20defined,output%20and%20calls%20abort()
.
Re: [DISCUSS] Default state of NDEBUG
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Feb 17, 2022 at 4:08 AM Petro Karashchenko
<pe...@gmail.com> wrote:
> My point about two config options was more because I think that apps
> and kernel are two separate entities and if there is a need to add
> extra debugging capabilities to the kernel it does not mean that it
> needs to be added for apps as well and vice versa. This seems to be
> right to me from the logical perspective especially if kernel mode
> build is selected. But going with a single option can be a solution as
> well.
Seems logical to me. More below:
> The other configuration options that you mention were mostly done to
> reach the tiny memory footprint rather than an attempt to violate
> POSIX. If we want to build a system that is capable of running from
> 8-bit to 64-bit embedded systems I think we can't afford to enable all
> the features by default.
This is my concern as well. <rant>The whole point of NuttX for me is
that it provides a way to use many different microcontrollers and
switch easily from one family to another. This happens all the time in
my $DAYJOB because often an existing design is being updated and the
customer wants some new feature, and the MCU is, say, Microchip, and
it doesn't have some peripheral we need, and another chip, say, STM32
does have it, so now we have to rewrite the entire firmware because we
wanted to add one feature. By using NuttX we just recompile the
firmware with minimal changes. Also we make a lot of software for the
PC side, so using NuttX allows us to share more code between PC and
embedded. That's made possible by NuttX being POSIX-like, but I like
that I can disable unneeded features to save FLASH and RAM which can
significantly reduce cost. And there are still a lot of products that
can use 8- or 16-bit MCUs. We should not forget where we come
from.</rant>
Cheers,
Nathan
Re: [DISCUSS] Default state of NDEBUG
Posted by Petro Karashchenko <pe...@gmail.com>.
Hello.
My point about two config options was more because I think that apps
and kernel are two separate entities and if there is a need to add
extra debugging capabilities to the kernel it does not mean that it
needs to be added for apps as well and vice versa. This seems to be
right to me from the logical perspective especially if kernel mode
build is selected. But going with a single option can be a solution as
well.
The other configuration options that you mention were mostly done to
reach the tiny memory footprint rather than an attempt to violate
POSIX. If we want to build a system that is capable of running from
8-bit to 64-bit embedded systems I think we can't afford to enable all
the features by default.
Best regards,
Petro
чт, 17 лют. 2022 р. о 10:52 Juha Niskanen (Haltian)
<ju...@haltian.com> пише:
>
> NDEBUG is just one macro name. I feel it is silly to have either one or two CONFIG macros to control whether or not it is defined or not. My preference is zero new CONFIG macros. We have too many already and trend seems to be remove ones that disable standard POSIX features (CONFIG_DISABLE_SIGNALS, CONFIG_DISABLE_POLL, CONFIG_CLOCK_MONOTONIC etc.)
>
> assert() and NDEBUG for apps should work exactly as in POSIX per Inviolables.
>
> If assert() is problem in kernel code, simply replace it with ASSERT() or DEBUGASSERT().
>
> Currently NuttX assert() macro expands into a C statement. This is incorrect, as C standard requires it to expand to a void expression (a minor conformance issue, most real programs won't notice the difference).
>
> -Juha
> ________________________________
> From: Petro Karashchenko <pe...@gmail.com>
> Sent: Wednesday, February 16, 2022 10:31 PM
> To: dev@nuttx.apache.org <de...@nuttx.apache.org>
> Subject: Re: [DISCUSS] Default state of NDEBUG
>
> I'm not sure that I'm fully following the discussion (I will read the PR
> comments to get the full context), but my vote is for:
> 1. There should be a separate way to build kernel and app with assert()
> enabled.
> 2. The assert() should be disabled by default. So the default build is a
> release build.
> 3. I do not care if it is a config option or I need to "make NDEBUG=1" from
> a command line. Probably 2 Kconfig options (one for kernel and 1 for apps
> would be the best).
>
> Best regards,
> Petro
>
> On Mon, Feb 14, 2022, 7:56 PM David Sidrane <da...@nscdg.com> wrote:
>
> > PR 5399 adds an Kconfig option for NDEBUG. The salient discussion begins at
> > [2] there are mixed positions and reasoning. xiaoxiang781216 asked me to
> > raise a discussion on this.
> >
> >
> >
> > The reasoning for the Default state of to be NDEBUG (n) hence undefined so
> > that assert() enabled is the following:
> >
> >
> >
> > 1) It follows the standard understanding of NDEBUG
> >
> >
> >
> > The standard for standard library from [3]
> >
> >
> >
> > The definition of the macro assert depends on another macro,
> > NDEBUG, *which is not defined* by the standard library.
> >
> >
> >
> >
> >
> > 2) We have DEBUGASSERT for use in the OS. I believe this was an
> > intentional separation on Greg’s part. We have asked for is input.
> >
> >
> >
> > In a NuttX "Release" build DEGUASSERT is off (all debug is off to show off
> > the build size).
> >
> > I should still be able to build the app code with assert() and not have to
> > use a Kconfig to enable it.
> >
> > How would you prefer it to be defined?
> >
> >
> >
> > 1. Defaulted ON – assert() is a No OP
> > 2. Defaulted OFF assert() is enabled.
> > 3. Left to a command line setting from build system
> >
> >
> >
> > David
> >
> >
> >
> > [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-nuttx%2Fpull%2F5399&data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=PfBujKU%2BW4LlfyvqZPmPy6rpUIbhWzTJpb93vs%2FbPic%3D&reserved=0
> >
> >
> >
> > [2]
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-nuttx%2Fpull%2F5399%23issuecomment-1029387606&data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=a62%2FME0f8Iq1mMFqRdqqokm1EyuhVplv3r3RkQQjQTo%3D&reserved=0
> >
> > [3]
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.cppreference.com%2Fw%2Fc%2Ferror%2Fassert%23%3A~%3Atext%3DIf%2520NDEBUG%2520is%2520defined%2520as%2Cthe%2520source%2520code%2520where%2520%253Cassert.%26text%3DIf%2520NDEBUG%2520is%2520not%2520defined%2Coutput%2520and%2520calls%2520abort&data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=kpoMBy%2BnLyiptB2GHnNBIS9Ln%2FEv7tn64tegKeV%2B3Nw%3D&reserved=0()
> > .
> >
Re: [DISCUSS] Default state of NDEBUG
Posted by "Juha Niskanen (Haltian)" <ju...@haltian.com>.
NDEBUG is just one macro name. I feel it is silly to have either one or two CONFIG macros to control whether or not it is defined or not. My preference is zero new CONFIG macros. We have too many already and trend seems to be remove ones that disable standard POSIX features (CONFIG_DISABLE_SIGNALS, CONFIG_DISABLE_POLL, CONFIG_CLOCK_MONOTONIC etc.)
assert() and NDEBUG for apps should work exactly as in POSIX per Inviolables.
If assert() is problem in kernel code, simply replace it with ASSERT() or DEBUGASSERT().
Currently NuttX assert() macro expands into a C statement. This is incorrect, as C standard requires it to expand to a void expression (a minor conformance issue, most real programs won't notice the difference).
-Juha
________________________________
From: Petro Karashchenko <pe...@gmail.com>
Sent: Wednesday, February 16, 2022 10:31 PM
To: dev@nuttx.apache.org <de...@nuttx.apache.org>
Subject: Re: [DISCUSS] Default state of NDEBUG
I'm not sure that I'm fully following the discussion (I will read the PR
comments to get the full context), but my vote is for:
1. There should be a separate way to build kernel and app with assert()
enabled.
2. The assert() should be disabled by default. So the default build is a
release build.
3. I do not care if it is a config option or I need to "make NDEBUG=1" from
a command line. Probably 2 Kconfig options (one for kernel and 1 for apps
would be the best).
Best regards,
Petro
On Mon, Feb 14, 2022, 7:56 PM David Sidrane <da...@nscdg.com> wrote:
> PR 5399 adds an Kconfig option for NDEBUG. The salient discussion begins at
> [2] there are mixed positions and reasoning. xiaoxiang781216 asked me to
> raise a discussion on this.
>
>
>
> The reasoning for the Default state of to be NDEBUG (n) hence undefined so
> that assert() enabled is the following:
>
>
>
> 1) It follows the standard understanding of NDEBUG
>
>
>
> The standard for standard library from [3]
>
>
>
> The definition of the macro assert depends on another macro,
> NDEBUG, *which is not defined* by the standard library.
>
>
>
>
>
> 2) We have DEBUGASSERT for use in the OS. I believe this was an
> intentional separation on Greg’s part. We have asked for is input.
>
>
>
> In a NuttX "Release" build DEGUASSERT is off (all debug is off to show off
> the build size).
>
> I should still be able to build the app code with assert() and not have to
> use a Kconfig to enable it.
>
> How would you prefer it to be defined?
>
>
>
> 1. Defaulted ON – assert() is a No OP
> 2. Defaulted OFF assert() is enabled.
> 3. Left to a command line setting from build system
>
>
>
> David
>
>
>
> [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-nuttx%2Fpull%2F5399&data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=PfBujKU%2BW4LlfyvqZPmPy6rpUIbhWzTJpb93vs%2FbPic%3D&reserved=0
>
>
>
> [2]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-nuttx%2Fpull%2F5399%23issuecomment-1029387606&data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=a62%2FME0f8Iq1mMFqRdqqokm1EyuhVplv3r3RkQQjQTo%3D&reserved=0
>
> [3]
>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.cppreference.com%2Fw%2Fc%2Ferror%2Fassert%23%3A~%3Atext%3DIf%2520NDEBUG%2520is%2520defined%2520as%2Cthe%2520source%2520code%2520where%2520%253Cassert.%26text%3DIf%2520NDEBUG%2520is%2520not%2520defined%2Coutput%2520and%2520calls%2520abort&data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=kpoMBy%2BnLyiptB2GHnNBIS9Ln%2FEv7tn64tegKeV%2B3Nw%3D&reserved=0()
> .
>
Re: [DISCUSS] Default state of NDEBUG
Posted by Petro Karashchenko <pe...@gmail.com>.
I'm not sure that I'm fully following the discussion (I will read the PR
comments to get the full context), but my vote is for:
1. There should be a separate way to build kernel and app with assert()
enabled.
2. The assert() should be disabled by default. So the default build is a
release build.
3. I do not care if it is a config option or I need to "make NDEBUG=1" from
a command line. Probably 2 Kconfig options (one for kernel and 1 for apps
would be the best).
Best regards,
Petro
On Mon, Feb 14, 2022, 7:56 PM David Sidrane <da...@nscdg.com> wrote:
> PR 5399 adds an Kconfig option for NDEBUG. The salient discussion begins at
> [2] there are mixed positions and reasoning. xiaoxiang781216 asked me to
> raise a discussion on this.
>
>
>
> The reasoning for the Default state of to be NDEBUG (n) hence undefined so
> that assert() enabled is the following:
>
>
>
> 1) It follows the standard understanding of NDEBUG
>
>
>
> The standard for standard library from [3]
>
>
>
> The definition of the macro assert depends on another macro,
> NDEBUG, *which is not defined* by the standard library.
>
>
>
>
>
> 2) We have DEBUGASSERT for use in the OS. I believe this was an
> intentional separation on Greg’s part. We have asked for is input.
>
>
>
> In a NuttX "Release" build DEGUASSERT is off (all debug is off to show off
> the build size).
>
> I should still be able to build the app code with assert() and not have to
> use a Kconfig to enable it.
>
> How would you prefer it to be defined?
>
>
>
> 1. Defaulted ON – assert() is a No OP
> 2. Defaulted OFF assert() is enabled.
> 3. Left to a command line setting from build system
>
>
>
> David
>
>
>
> [1] https://github.com/apache/incubator-nuttx/pull/5399
>
>
>
> [2]
> https://github.com/apache/incubator-nuttx/pull/5399#issuecomment-1029387606
>
> [3]
>
> https://en.cppreference.com/w/c/error/assert#:~:text=If%20NDEBUG%20is%20defined%20as,the%20source%20code%20where%20%3Cassert.&text=If%20NDEBUG%20is%20not%20defined,output%20and%20calls%20abort()
> .
>