| Message ID | 20210601143422.25064-3-patrickdepinguin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Introduce BR2_ENABLE_RUNTIME_DEBUG | expand |
Thomas, All, On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly: > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if > it is set, then the assert statement is compiled away. > > Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the > default case). > > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr> However, see a little nit, below... > --- > v4: reintroduced patch on suggestion of Arnout. I had previously dropped it from > the series based on an incorrect conclusion. > > > package/Makefile.in | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/package/Makefile.in b/package/Makefile.in > index 86db62ba5b..955e6a8e8c 100644 > --- a/package/Makefile.in > +++ b/package/Makefile.in > @@ -148,6 +148,9 @@ endif > ifeq ($(BR2_DEBUG_3),y) > TARGET_DEBUGGING = -g3 Here, we have a simple assignment in a conditional block, while... > endif > +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),) > +TARGET_DEBUGGING += -DNDEBUG ... here we have an append-assignment in a conditional. In this case the order ensures it works, but this is inconsistent and prone to errors should we need to expand TARGET_DEBUGGING futher... Regards, Yann E. MORIN. > +endif > > TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS)) > > -- > 2.26.3 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Thomas, Yann, all, > On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly: > +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),) > +TARGET_DEBUGGING += -DNDEBUG This broke bitcoin builds. I haven't looked deeply into the details, but it is my understanding that NDEBUG changes the behaviour of assert() in a way that Bitcoin explicitly does not support. See: https://github.com/bitcoin/bitcoin/blob/707ba8692b0013f4824dc3c2ea6554ccad5d20c2/src/compat/assumptions.h#L13 I don't know if there is a workaround for this, but I have a feeling that passing NDEBUG for all packages might not be a good idea? --- D. Olsson PGP: 8204A8CD
Hello, On Sat, Jun 5, 2021, 18:50 D. Olsson <hi@senzilla.io> wrote: > Hi Thomas, Yann, all, > > > On 2021-06-01 16:34 +0200, Thomas De Schampheleire spake thusly: > > +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),) > > +TARGET_DEBUGGING += -DNDEBUG > > This broke bitcoin builds. I haven't looked deeply into the > details, but it is my understanding that NDEBUG changes the > behaviour of assert() in a way that Bitcoin explicitly does > not support. > > See: > https://github.com/bitcoin/bitcoin/blob/707ba8692b0013f4824dc3c2ea6554ccad5d20c2/src/compat/assumptions.h#L13 > > I don't know if there is a workaround for this, but I have a > feeling that passing NDEBUG for all packages might not be a > good idea? > NDEBUG is a macro that, when set, will neutralize assert statements (see man assert). The rationale is that assert statements can help developers validate assumptions in a test phase, but that this code is better disabled in release builds, for reasons of performance. At that time, the code is expected to be correct. It's odd but not unacceptable that bitcoin expects NDEBUG not to be set. The workaround/solution would be to explicitly undefine NDEBUG, via -UNDEBUG. This should become part of the flags set in bitcoin.mk on Buildroot. It is possible that there are other packages that need this change, but generally one wpuld expect projects to allow both scenarios. Best regards Thomas
Hello, On Tue, 1 Jun 2021 16:34:07 +0200 Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if > it is set, then the assert statement is compiled away. > > Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the > default case). > > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> So, I would like to challenge whether this change is really a good idea. Since this has been merged, we had to add a significant number of patches to undefine NDEBUG. The manpage of assert() tells us: If the macro NDEBUG is defined at the moment <assert.h> was last in‐ cluded, the macro assert() generates no code, and hence does nothing at all. It is not recommended to define NDEBUG if using assert() to de‐ tect error conditions since the software may behave non-deterministi‐ cally. So there is a clear recommandation to *not* define NDEBUG if assert is used to detect error conditions. And later on, it also says: BUGS assert() is implemented as a macro; if the expression tested has side- effects, program behavior will be different depending on whether NDEBUG is defined. This may create Heisenbugs which go away when debugging is turned on. We have catched a number of packages where defining NDEBUG is causing build issues, but I am worried about those packages where it doesn't cause build issues, but assert() is used to run something that has side-effects, and where the issue would only be visible at runtime. Yes, ideally, assert() should only be used to verify expressions that have no side effects. But practically speaking, I am not sure how many programmers are really aware of the fact that assert() expressions should not have side effects, because assert() can be disabled by defining NDEBUG. I believe this change is too dangerous, has caused a number of build breakage + can cause run-time breakage that is difficult to predict. What do you think ? Thomas
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes: > Hello, > On Tue, 1 Jun 2021 16:34:07 +0200 > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: >> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> >> >> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if >> it is set, then the assert statement is compiled away. >> >> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the >> default case). >> >> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > So, I would like to challenge whether this change is really a good > idea. Since this has been merged, we had to add a significant number of > patches to undefine NDEBUG. Indeed. > I believe this change is too dangerous, has caused a number of build > breakage + can cause run-time breakage that is difficult to predict. > What do you think ? I agree, I don't think it ends up being a net positive regarding cost/benefit. I would prefer to revert it.
Peter, Thomas², All, On 2021-07-04 13:01 +0200, Peter Korsgaard spake thusly: > >>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes: > > Hello, > > On Tue, 1 Jun 2021 16:34:07 +0200 > > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > >> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > >> > >> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if > >> it is set, then the assert statement is compiled away. > >> > >> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the > >> default case). > >> > >> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > So, I would like to challenge whether this change is really a good > > idea. Since this has been merged, we had to add a significant number of > > patches to undefine NDEBUG. > Indeed. Compiling without assert() should never have had an impact on the behaviour of the program. Semantically, assert() should only be used to test pre- and post-conditions (e.g. a value we were passed is in an acceptable range), i.e. to ensure the contract of the API. The problem is that developpers have started using assert() as a mean to test expected error conditions (e.g. the socket was closed by the remote end), instead of handling them gracefully. As such, the semantic of assert() has shifted enough that the original intent is mush less prominent than what it is currently used for. So yes, this does more harm than it brings benefits. > > I believe this change is too dangerous, has caused a number of build > > breakage + can cause run-time breakage that is difficult to predict. > > What do you think ? > I agree, I don't think it ends up being a net positive regarding > cost/benefit. I would prefer to revert it. In the end, for packages that do use assert() can add NDEBUG to their CFLAGS if so they wish. I will revert all those changes, since I was the one who applied the original changes. Regards, Yann E. MORIN.
Peter, Thomas², All, On 2021-07-04 13:32 +0200, Yann E. MORIN spake thusly: > On 2021-07-04 13:01 +0200, Peter Korsgaard spake thusly: > > >>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes: > > > On Tue, 1 Jun 2021 16:34:07 +0200 > > > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > > >> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > >> > > >> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if > > >> it is set, then the assert statement is compiled away. > > >> > > >> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the > > >> default case). > > >> > > >> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > > So, I would like to challenge whether this change is really a good > > > idea. Since this has been merged, we had to add a significant number of > > > patches to undefine NDEBUG. "Significant", being just 2 packages I had to revert as they do not have corresponding upstream changes, or upstream expressly refused to support NDEBUGi: bitcoin and weston. A few others have had the NDEBUG changes accepted upstream, and for some wee even have updated to a version with those patches. Plus 6 patches pending in patchwork: - gcc: the issue is not trivial: -Werror or -DNDEBUG? - sane-backends: merged upstream - tor: can't build with NDEBUG - casync: can't with NDEBUG - qemu: can't with NDEBUG - xen: is already using NDEBUG, but causes -Werror issues So, that's 8 packages affected by NDEBUG, of which 6 or 7 are going to be an issue. I would not call that 'significant' yet. However, as I said earlier, I still think this is not worth the effort, indeed. Regards, Yann E. MORIN.
On 04/07/2021 11:15, Thomas Petazzoni wrote: > Hello, > > On Tue, 1 Jun 2021 16:34:07 +0200 > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > >> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> >> >> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if >> it is set, then the assert statement is compiled away. >> >> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the >> default case). >> >> Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > So, I would like to challenge whether this change is really a good > idea. I'm the one who originally proposed this (I think) so I better pipe up. > Since this has been merged, we had to add a significant number of > patches to undefine NDEBUG. > > The manpage of assert() tells us: > > If the macro NDEBUG is defined at the moment <assert.h> was last in‐ > cluded, the macro assert() generates no code, and hence does nothing at > all. It is not recommended to define NDEBUG if using assert() to de‐ > tect error conditions since the software may behave non-deterministi‐ > cally. In practice, the build failures are mostly due to variables that become unused because the assert() is gone, and that triggers -Werror. > So there is a clear recommandation to *not* define NDEBUG if assert is > used to detect error conditions. And later on, it also says: > > BUGS > assert() is implemented as a macro; if the expression tested has side- > effects, program behavior will be different depending on whether NDEBUG > is defined. This may create Heisenbugs which go away when debugging is > turned on. > > We have catched a number of packages where defining NDEBUG is causing > build issues, but I am worried about those packages where it doesn't > cause build issues, but assert() is used to run something that has > side-effects, and where the issue would only be visible at runtime. I don't think it's very likely that this happens, but indeed it's possibly. You just have to "optimise": bool result = f(); assert(result); into assert(f()); and you get hit by this issue... > Yes, ideally, assert() should only be used to verify expressions that > have no side effects. But practically speaking, I am not sure how many > programmers are really aware of the fact that assert() expressions > should not have side effects, because assert() can be disabled by > defining NDEBUG. Well, packages that use meson or CMake *do* get NDEBUG set (because that's done by the release variants), so those packages at least seem to behave sanely... That said: > I believe this change is too dangerous, This is definitely true. So I also think reverting it is the safest option. Regards, Arnout > has caused a number of build > breakage + can cause run-time breakage that is difficult to predict. > > What do you think ? > > Thomas >
Arnout, Peter, Thomas², All, On 2021-07-04 16:58 +0200, Arnout Vandecappelle spake thusly: > On 04/07/2021 11:15, Thomas Petazzoni wrote: > > On Tue, 1 Jun 2021 16:34:07 +0200 > > Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote: > >> From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > >> The 'assert' statement in glibc honors the 'NDEBUG' preprocessor macro: if > >> it is set, then the assert statement is compiled away. > >> Define this 'NDEBUG' macro when BR2_ENABLE_RUNTIME_DEBUG is disabled (the > >> default case). > > So, I would like to challenge whether this change is really a good > > idea. > In practice, the build failures are mostly due to variables that become unused > because the assert() is gone, and that triggers -Werror. [--SNIP--] > I don't think it's very likely that this happens, but indeed it's possibly. You > just have to "optimise": > bool result = f(); > assert(result); > into > assert(f()); > and you get hit by this issue... And this is way too easy an optimisation to do. ;-) > > Yes, ideally, assert() should only be used to verify expressions that > > have no side effects. But practically speaking, I am not sure how many > > programmers are really aware of the fact that assert() expressions > > should not have side effects, because assert() can be disabled by > > defining NDEBUG. > Well, packages that use meson or CMake *do* get NDEBUG set (because that's done > by the release variants), so those packages at least seem to behave sanely... > That said: > > > I believe this change is too dangerous, > > This is definitely true. So I also think reverting it is the safest option. I've now done so: I reverted the change itself, and reverted the two packages that had workarounds for this; fc0fb99f0307 Revert "package/weston: disable -NDEBUG" 1aa3cb109067 Revert "package/bitcoin: unset the NDEBUG flag" a1c7cff1a081 Revert "core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set" The other NDEBUG-related changes, I have left in the tree: b7939fe2a0b1 core: introduce BR2_ENABLE_RUNTIME_DEBUG => used by meson and cmake 61c5e0319c5 package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on BR2_ENABLE_RUNTIME_DEBUG => see commit log for details fb12adbb761 package/sofia-sip: correct passing of '--enable-ndebug' 0993954814e package/sofia-sip: don't set 'NDEBUG' explicitly => the first was a fix; the second drops an explicit handling maybe we could revert 0993954814e, but I don't see the point ea02ac3e061 package/daemon: fix build without BR2_ENABLE_RUNTIME_DEBUG => very similar fix accepted upstream, so we will drop the patch when we bump bf32928bbb0 package/gnutls: disable tests => we still want to disable tests even without NDEBUG 1f1d536e7e3 package/pdbg: fix build with -DNDEBUG 932f6a0a2ad package/pdbg: Bump version to v3.3 => a patch we had was accepted upstream, and we updated the version cc1c8c3bb1c package/openswan: disable -Werror => we still want to disable Werror even without NDEBUG 12551386027 package/filemq: bump to af4768dcaf2fcb8083a32bad107a22ecb7a5d954 => is a version bump anyway I will now reply to the individual pending patches telated to NDEBUG, and reject them. Regards, Yann E. MORIN.
diff --git a/package/Makefile.in b/package/Makefile.in index 86db62ba5b..955e6a8e8c 100644 --- a/package/Makefile.in +++ b/package/Makefile.in @@ -148,6 +148,9 @@ endif ifeq ($(BR2_DEBUG_3),y) TARGET_DEBUGGING = -g3 endif +ifeq ($(BR2_ENABLE_RUNTIME_DEBUG),) +TARGET_DEBUGGING += -DNDEBUG +endif TARGET_LDFLAGS = $(call qstrip,$(BR2_TARGET_LDFLAGS))