From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 6FCB13858407 for ; Tue, 27 Jul 2021 13:38:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6FCB13858407 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from imap1.suse-dmz.suse.de (imap1.suse-dmz.suse.de [192.168.254.73]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 9D00C2012E; Tue, 27 Jul 2021 13:38:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1627393100; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wF+J88Aroo5BjypCDZWK9Ix/Ey/onf0Wo4fOiXh8e1E=; b=yRLb/TJgGu9heaC8VtjWhDE+r2/NAnA6gXBsfvGWbUnIs3LeMgs0W7UY7ZvvymanYt+lTZ ++XzXsfTdjJJTifqjCBJgJEBRuY9+ff2tm3Bodyciia29kFN+q+0NfmMo6+oIq2l6cYJuS I0K4bAtqNjaHuhFidwUtIHhyQGyVwLU= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1627393100; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wF+J88Aroo5BjypCDZWK9Ix/Ey/onf0Wo4fOiXh8e1E=; b=hy/Fl/XRDs2Yb2sbxOPcEdr3qh55YLelGvYVa6wEAvgPqVlHIlhj0YOotyaeRBQXImqFt7 H/B51oWeOkBx71Dw== Received: from imap1.suse-dmz.suse.de (imap1.suse-dmz.suse.de [192.168.254.73]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap1.suse-dmz.suse.de (Postfix) with ESMTPS id 6E67013CF4; Tue, 27 Jul 2021 13:38:20 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap1.suse-dmz.suse.de with ESMTPSA id AiinGEwMAGGsfQAAGKfGzw (envelope-from ); Tue, 27 Jul 2021 13:38:20 +0000 Subject: Re: Building with recent GCC versions: gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] From: Tom de Vries To: Andrew Burgess Cc: Jan-Benedict Glaw , gdb@sourceware.org References: <20210726211101.ivychvbfgaafxjtz@lug-owl.de> <20210727100354.GB4037238@embecosm.com> <20210727113511.GC4037238@embecosm.com> <6cf80ba9-b010-bb42-c92d-84e4f396813c@suse.de> Message-ID: Date: Tue, 27 Jul 2021 15:38:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <6cf80ba9-b010-bb42-c92d-84e4f396813c@suse.de> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_ASCII_DIVIDERS, KAM_SHORT, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Jul 2021 13:38:23 -0000 On 7/27/21 1:49 PM, Tom de Vries wrote: > On 7/27/21 1:35 PM, Andrew Burgess wrote: >> * Tom de Vries [2021-07-27 12:44:10 +0200]: >> >>> On 7/27/21 12:03 PM, Andrew Burgess wrote: >>>> * Jan-Benedict Glaw [2021-07-26 23:11:01 +0200]: >>>> >>>>> Hi! >>>>> >>>>> I'm running some CI builds and noticed that, when building GDB with >>>>> quite recent GCC versions, it breaks. >>>>> >>>>> With ie. this "gcc-snapshot" GCC from Debian: >>>>> >>>>> /usr/lib/gcc-snapshot/bin/gcc --version >>>>> gcc (Debian 20210630-1) 12.0.0 20210630 (experimental) [master revision 6bf383c37e6:93c270320bb:35da8a98026849bd20d16bbf9210ac1d0b44ea6a] >>>>> >>>>> we see: >>>>> >>>>> ./configure --target=i686-linux --prefix=/tmp/gdb-i686-linux >>>>> [...] >>>>> all make V=1 all-gdb >>>>> [...] >>>>> [all 2021-07-26 20:39:22] /usr/lib/gcc-snapshot/bin/g++ -x c++ -I. -I. -I./config -DLOCALEDIR="\"/tmp/gdb-i686-linux/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../readline/readline/.. -I./../zlib -I../bfd -I./../bfd -I./../include -I../libdecnumber -I./../libdecnumber -I./../gnulib/import -I../gnulib/import -I./.. -I.. -DTUI=1 -I./.. -pthread -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-variable -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wunused-but-set-parameter -Wunused-but-set-variable -Wno-sign-compare -Wno-error=maybe-uninitialized -Wno-mismatched-tags -Wsuggest-override -Wimplicit-fallthrough=3 -Wduplicated-cond -Wshadow=local -Wdeprecated-copy -Wdeprecated-copy-dtor -Wredundant-move -Wmissing-declarations -Wstrict-null-sentinel -Wformat -Wformat-nonliteral -Werror -g -O2 -c -o compile/compile.o -MT compile/compile.o -MMD -MP -MF compile/.deps/compile.Tpo compile/compile.c >>>>> [all 2021-07-26 20:39:26] In file included from ./../gdbsupport/common-defs.h:126, >>>>> [all 2021-07-26 20:39:26] from ./defs.h:28, >>>>> [all 2021-07-26 20:39:26] from compile/compile.c:20: >>>>> [all 2021-07-26 20:39:26] ./../gdbsupport/gdb_unlinker.h: In constructor 'gdb::unlinker::unlinker(const char*)': >>>>> [all 2021-07-26 20:39:26] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull' argument 'filename' compared to NULL [-Werror=nonnull-compare] >>>>> [all 2021-07-26 20:39:26] 35 | ((void) ((expr) ? 0 : \ >>>>> [all 2021-07-26 20:39:26] | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> [all 2021-07-26 20:39:26] 36 | (gdb_assert_fail (#expr, __FILE__, __LINE__, FUNCTION_NAME), 0))) >>>>> [all 2021-07-26 20:39:26] | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> [all 2021-07-26 20:39:26] ./../gdbsupport/gdb_unlinker.h:38:5: note: in expansion of macro 'gdb_assert' >>>>> [all 2021-07-26 20:39:26] 38 | gdb_assert (filename != NULL); >>>>> [all 2021-07-26 20:39:26] | ^~~~~~~~~~ >>>>> [all 2021-07-26 20:39:27] cc1plus: all warnings being treated as errors >>>>> [all 2021-07-26 20:39:27] make[1]: *** [Makefile:1642: compile/compile.o] Error 1 >>>>> [all 2021-07-26 20:39:27] make[1]: Leaving directory '/var/lib/laminar/run/gdb-i686-linux/4/binutils-gdb/gdb' >>>>> [all 2021-07-26 20:39:27] make: *** [Makefile:11410: all-gdb] Error 2 >>>>> >>>>> >>>>> I also discussed this on the GCC patches mailing list >>>>> (https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575568.html), >>>>> where Martin suggested that this should be fixed here in GDB. >>>>> >>>>> Any thoughts about this? >>>> >>>> As I understand it the nonnull attribute only provides compile time >>>> protection against explicitly passing NULL, there's no compiled in >>>> non-null check (well, maybe with -fisolate-erroneous-paths-attribute, >>>> but the assert might give a better error message). >>>> >>>> This means its still possible to pass NULL to a nonnull function, its >>>> just the behaviour of the program is undefined in that case. >>>> >>>> So, it doesn't seem crazy that we might want to both (a) have a >>>> function declared nonnull, to prevent explicitly passing NULL, and (b) >>>> have a NULL check inside the function to catch logic bugs that result >>>> in NULL being passed. >>>> >>>> We could, of course, push the assert outside of the function, but that >>>> would really suck due to code duplication, and the risk of missing an >>>> assert, so that seems like a non-starter. >>>> >>>> We could drop either the assert, or the nonnull attribute, but that >>>> would suck as both give a valuable, but different form of protection. >>>> >>>> After some experimenting, I suspect that the assert is being optimised >>>> away anyway, which kind of makes sense, as we're telling the compiler >>>> it can assume that the pointer is non-null. >>>> >>> >>> Yes, in fact that's what the nonnull-compare warning specifically warns >>> against: there's some code that may be optimized away, due to the >>> nonnull attribute. >>> >>>> So, what we probably want is someway to tell (or trick) GCC into >>>> including the null check even in the nonnull function.... >>>> >>>> ... here's what I came up with, add this somewhere: >>>> >>>> template >>>> bool __attribute__ ((noinline)) >>>> nonnull_arg_is_really_not_null (const T *ptr) >>>> { >>>> return ptr != nullptr; >>>> } >>>> >>>> then change the assert to: >>>> >>>> gdb_assert (nonnull_arg_is_really_not_null (filename)); >>>> >>>> Seems to keep the assert, and silence the warning. Thoughts? >>>> >>> >>> I understand why it works, but it seems fragile to me. At some point >>> some compiler may get smart enough to also optimize this case, and then >>> we're back in the same situation. >> >> Good point. >> >> The GCC documentation for noinline[1] suggests we can avoid the call >> being removed by adding 'asm ("");' into the function: >> >> template >> bool __attribute__ ((noinline)) >> nonnull_arg_is_really_not_null (const T *ptr) >> { >> asm (""); >> return ptr != nullptr; >> } >> >> I'm not really arguing for this approach over any other, just sharing >> what I discovered. >> > > Ack, understood. Note that the added asm doesn't stop a compiler from > doing: > ... > gdb_assert (nonnull_arg_is_really_not_null (filename)); > ... > -> > ... > nonnull_arg_is_really_not_null (filename); > gdb_assert (true); > ... > > Thanks, > - Tom > >> Thanks, >> Andrew >> >> [1] https://gcc.gnu.org/onlinedocs/gcc-11.1.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes >> >>> >>> I wonder whether using volatile is a better idea (can't try this out >>> right now). >>> I was thinking of something like this: ... diff --git a/gdbsupport/gdb_unlinker.h b/gdbsupport/gdb_unlinker.h index bda6fe7ab54..3d99b41e7ad 100644 --- a/gdbsupport/gdb_unlinker.h +++ b/gdbsupport/gdb_unlinker.h @@ -20,6 +20,13 @@ #ifndef COMMON_GDB_UNLINKER_H #define COMMON_GDB_UNLINKER_H +template +const T *volatile +ignore_nonnull (const T *ptr) +{ + return ptr; +} + namespace gdb { @@ -35,7 +42,7 @@ class unlinker unlinker (const char *filename) ATTRIBUTE_NONNULL (2) : m_filename (filename) { - gdb_assert (filename != NULL); + gdb_assert (ignore_nonnull (filename) != NULL); } ~unlinker () ... This builds for me, though I haven't got a setup yet where the warning reproduces, so I can't check whether it actually fixes things. Thanks, - Tom