public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Carlos O'Donell <carlos@redhat.com>
To: Florian Weimer <fweimer@redhat.com>
Cc: libc-alpha <libc-alpha@sourceware.org>
Subject: [PATCH v2] Fix failure when CFLAGS contains -DNDEBUG (Bug 25251)
Date: Wed, 04 Dec 2019 16:49:00 -0000	[thread overview]
Message-ID: <e496f426-102a-5f85-9d1c-7ab750a17ff8@redhat.com> (raw)
In-Reply-To: <35485007-9fa2-30d8-2a28-3f6ad577ee8e@redhat.com>

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

      reply	other threads:[~2019-12-04 16:49 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-03 20:47 [PATCH] Fix build with -march=486 -Os (Bug 25240) Carlos O'Donell
2019-12-03 21:41 ` Florian Weimer
2019-12-03 22:01   ` [PATCH v2] Fix build with -march=486 -Os -DNDEBUG " Carlos O'Donell
2019-12-04  7:31     ` Florian Weimer
2019-12-04 14:44       ` [PATCH] Fix failure to run tests when CFLAGS contains -DNDEBUG (Bug 25251) Carlos O'Donell
2019-12-04 16:49         ` Carlos O'Donell [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e496f426-102a-5f85-9d1c-7ab750a17ff8@redhat.com \
    --to=carlos@redhat.com \
    --cc=fweimer@redhat.com \
    --cc=libc-alpha@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).