[PATCH] Fix build with -march=486 -Os (Bug 25240)

[PATCH] Fix build with -march=486 -Os (Bug 25240)

[Carlos O'Donell]

While reviewing bug 25240 I found that -march=486 -Os doesn't build
with gcc 9.2.1 because of this problem. Fixing is straight forward,
but I'm not sure if undefining NDEBUG like this is the best solution.
I wonder if there isn't a pragma we might use here? Thoughts?

8< --- 8< --- 8<
In a -march=486 -Os build gcc 9.2.1 issues the following error:
tst-assert-c++.cc: In function ‘int do_test()’:
tst-assert-c++.cc:66:12: error: unused variable ‘value’ [-Werror=unused-variable]
   66 |     no_int value;
      |            ^~~~~
tst-assert-c++.cc:71:18: error: unused variable ‘value’ [-Werror=unused-variable]
   71 |     bool_and_int value;
      |                  ^~~~~
The assert has been disabled by -Os leaving the value unused.
We never want the assert disabled because that's the point of the
test, so we undefine NDEBUG before including assert.h to ensure that
we get assert defined correctly.
---
 assert/tst-assert-c++.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/assert/tst-assert-c++.cc b/assert/tst-assert-c++.cc
index 41cb487512..a175f5e961 100644
--- a/assert/tst-assert-c++.cc
+++ b/assert/tst-assert-c++.cc
@@ -16,6 +16,9 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+/* We do not want the compiler or any other pre-included header from
+   removing the assert we want to test, so undefine NDEBUG right now.  */
+#undef NDEBUG
 #include <assert.h>
 
 /* The C++ standard requires that if the assert argument is a constant
-- 
2.21.0

Re: [PATCH] Fix build with -march=486 -Os (Bug 25240)

[Florian Weimer]

* Carlos O'Donell:

> While reviewing bug 25240 I found that -march=486 -Os doesn't build
> with gcc 9.2.1 because of this problem. Fixing is straight forward,
> but I'm not sure if undefining NDEBUG like this is the best solution.
> I wonder if there isn't a pragma we might use here? Thoughts?

-march=i486?

Where does this NDEBUG come from, exactly?

Thanks,
Florian

[PATCH v2] Fix build with -march=486 -Os -DNDEBUG (Bug 25240)

[Carlos O'Donell]

On 12/3/19 4:41 PM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> While reviewing bug 25240 I found that -march=486 -Os doesn't build
>> with gcc 9.2.1 because of this problem. Fixing is straight forward,
>> but I'm not sure if undefining NDEBUG like this is the best solution.
>> I wonder if there isn't a pragma we might use here? Thoughts?
> 
> -march=i486?

Sorry, yes.

> Where does this NDEBUG come from, exactly?

Sorry this is confusing, let me rewrite the commit message since I see
what I wrote was confusing.

In this case it comes directly from CFLAGS, and CXXFLAGS, and CPPFLAGS
used to build glibc for as small a size as possible.

Neither the compiler nor the headers inject it for -Os, rather I do
in order to get as small a glibc as possible. I wrote the commit message
rather vaguely and in the abstract sense that anything might inject a
-DNDEBUG, but let me be concrete here.

Since we don't build Fedora with -DNDEBUG anymore we don't see these
issues, but -marhc=i486 -Os -DNDEBUG builds do (small size).

How is this v2 with a better commit message?

8< --- 8< --- 8<
In a -march=486 -Os build gcc 9.2.1 issues the following error:
tst-assert-c++.cc: In function ‘int do_test()’:
tst-assert-c++.cc:66:12: error: unused variable ‘value’ [-Werror=unused-variable]
   66 |     no_int value;
      |            ^~~~~
tst-assert-c++.cc:71:18: error: unused variable ‘value’ [-Werror=unused-variable]
   71 |     bool_and_int value;
      |                  ^~~~~

The assert has been disabled by building glibc with CFLAGS, CXXFLAGS,
and CPPFLAGS with -DNDEBUG which removes the assert (minimum size)
and leaves the value unused.

We never want the assert disabled because that's the point of the
test, so we undefine NDEBUG before including assert.h to ensure that
we get assert defined correctly.
---
 assert/tst-assert-c++.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/assert/tst-assert-c++.cc b/assert/tst-assert-c++.cc
index 41cb487512..a175f5e961 100644
--- a/assert/tst-assert-c++.cc
+++ b/assert/tst-assert-c++.cc
@@ -16,6 +16,9 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+/* We do not want the compiler or any other pre-included header from
+   removing the assert we want to test, so undefine NDEBUG right now.  */
+#undef NDEBUG
 #include <assert.h>
 
 /* The C++ standard requires that if the assert argument is a constant
-- 
2.21.0

Re: [PATCH v2] Fix build with -march=486 -Os -DNDEBUG (Bug 25240)

[Florian Weimer]

* Carlos O'Donell:

> How is this v2 with a better commit message?

Sorry, why has -DNDEBUG got anything with -march=i486 (sic) or -Os?
I think you should mention -DNDEBUG only.

The comment should probably mention CFLAGS and -DNDEBUG, because that's
the common case.

Thanks,
Florian

[PATCH] Fix failure to run tests when CFLAGS contains -DNDEBUG (Bug 25251)

[Carlos O'Donell]

On 12/4/19 2:31 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> How is this v2 with a better commit message?
> 
> Sorry, why has -DNDEBUG got anything with -march=i486 (sic) or -Os?
> I think you should mention -DNDEBUG only.
> 
> The comment should probably mention CFLAGS and -DNDEBUG, because that's
> the common case.

Ah, I see your point. Cleaned up. I filed a new bug for this user-visible
build bug. The following patch fixes the issue, and commit message update.

No regressions on a build with -DNDEBUG.

OK for master?

From 080292f03e7489910750d9fe79e652aa8636bc84 Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Tue, 3 Dec 2019 15:42:24 -0500
Subject: [PATCH] Fix failure to run tests when CFLAGS contains -DNDEBUG (Bug
 25251)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Building tests with -DNDEBUG in CFLAGS, gcc 9.2.1 issues the following error:
tst-assert-c++.cc: In function ‘int do_test()’:
tst-assert-c++.cc:66:12: error: unused variable ‘value’ [-Werror=unused-variable]
   66 |     no_int value;
      |            ^~~~~
tst-assert-c++.cc:71:18: error: unused variable ‘value’ [-Werror=unused-variable]
   71 |     bool_and_int value;
      |                  ^~~~~

The assert has been disabled by building glibc with CFLAGS, CXXFLAGS,
and CPPFLAGS with -DNDEBUG which removes the assert and leaves the
value unused.

We never want the assert disabled because that's the point of the
test, so we undefine NDEBUG before including assert.h to ensure that
we get assert correctly defined.
---
 assert/tst-assert-c++.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/assert/tst-assert-c++.cc b/assert/tst-assert-c++.cc
index 41cb487512..a175f5e961 100644
--- a/assert/tst-assert-c++.cc
+++ b/assert/tst-assert-c++.cc
@@ -16,6 +16,9 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+/* We do not want the compiler or any other pre-included header from
+   removing the assert we want to test, so undefine NDEBUG right now.  */
+#undef NDEBUG
 #include <assert.h>
 
 /* The C++ standard requires that if the assert argument is a constant
-- 
2.21.0

[PATCH v2] Fix failure when CFLAGS contains -DNDEBUG (Bug 25251)

[Carlos O'Donell]

On 12/4/19 9:44 AM, Carlos O'Donell wrote:
> On 12/4/19 2:31 AM, Florian Weimer wrote:
>> * Carlos O'Donell:
>>
>>> How is this v2 with a better commit message?
>>
>> Sorry, why has -DNDEBUG got anything with -march=i486 (sic) or -Os?
>> I think you should mention -DNDEBUG only.
>>
>> The comment should probably mention CFLAGS and -DNDEBUG, because that's
>> the common case.
> 
> Ah, I see your point. Cleaned up. I filed a new bug for this user-visible
> build bug. The following patch fixes the issue, and commit message update.
> 
> No regressions on a build with -DNDEBUG.
> 
> OK for master?

I realized the comment in the code should get cleaned up too based on
your comment.

v2.

OK for master?

8< --- 8< --- 8<
Building tests with -DNDEBUG in CFLAGS, gcc 9.2.1 issues the following error:
tst-assert-c++.cc: In function ‘int do_test()’:
tst-assert-c++.cc:66:12: error: unused variable ‘value’ [-Werror=unused-variable]
   66 |     no_int value;
      |            ^~~~~
tst-assert-c++.cc:71:18: error: unused variable ‘value’ [-Werror=unused-variable]
   71 |     bool_and_int value;
      |                  ^~~~~

The assert has been disabled by building glibc with CFLAGS, CXXFLAGS,
and CPPFLAGS with -DNDEBUG which removes the assert and leaves the
value unused.

We never want the assert disabled because that's the point of the
test, so we undefine NDEBUG before including assert.h to ensure that
we get assert correctly defined.
---
 assert/tst-assert-c++.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/assert/tst-assert-c++.cc b/assert/tst-assert-c++.cc
index 41cb487512..c01fc8bd25 100644
--- a/assert/tst-assert-c++.cc
+++ b/assert/tst-assert-c++.cc
@@ -16,6 +16,9 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+/* Undefine NDEBUG to ensure the build system e.g. CFLAGS/CXXFLAGS
+   does not disable the asserts we want to test.  */
+#undef NDEBUG
 #include <assert.h>
 
 /* The C++ standard requires that if the assert argument is a constant
-- 
2.21.0