Re: [DISCUSS] Default state of NDEBUG
On Thu, Feb 17, 2022 at 4:08 AM Petro Karashchenko 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. 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. Cheers, Nathan
Re: [DISCUSS] Default state of NDEBUG
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) пише: > > 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 > Sent: Wednesday, February 16, 2022 10:31 PM > To: dev@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 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%2F5399data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=PfBujKU%2BW4LlfyvqZPmPy6rpUIbhWzTJpb93vs%2FbPic%3Dreserved=0 > > > > > > > > [2] > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-nuttx%2Fpull%2F5399%23issuecomment-1029387606data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=a62%2FME0f8Iq1mMFqRdqqokm1EyuhVplv3r3RkQQjQTo%3Dreserv
Re: [DISCUSS] Default state of NDEBUG
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 Sent: Wednesday, February 16, 2022 10:31 PM To: dev@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 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%2F5399data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=PfBujKU%2BW4LlfyvqZPmPy6rpUIbhWzTJpb93vs%2FbPic%3Dreserved=0 > > > > [2] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fincubator-nuttx%2Fpull%2F5399%23issuecomment-1029387606data=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=a62%2FME0f8Iq1mMFqRdqqokm1EyuhVplv3r3RkQQjQTo%3Dreserved=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%2520abortdata=04%7C01%7Cjuha.niskanen%40haltian.com%7Cb69e018597b04f0f623608d9f18b7694%7C2f7c629267f24cc582be5d187e289ab2%7C1%7C0%7C637806403583675784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=kpoMBy%2BnLyiptB2GHnNBIS9Ln%2FEv7tn64tegKeV%2B3Nw%3Dreserved=0() > . >
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 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.=If%20NDEBUG%20is%20not%20defined,output%20and%20calls%20abort() > . >
[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.=If%20NDEBUG%20is%20not%20defined,output%20and%20calls%20abort() .