From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id HXk7DCiUAmF8PwAAWB0awg (envelope-from ) for ; Thu, 29 Jul 2021 07:42:32 -0400 Received: by simark.ca (Postfix, from userid 112) id 256801E4A3; Thu, 29 Jul 2021 07:42:32 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.5 required=5.0 tests=DKIM_SIGNED, MAILING_LIST_MULTI,RDNS_DYNAMIC,T_DKIM_INVALID,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 94DFA1E4A3 for ; Thu, 29 Jul 2021 07:42:30 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 90827393AC33 for ; Thu, 29 Jul 2021 11:42:29 +0000 (GMT) Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 4AA7E3853821 for ; Thu, 29 Jul 2021 11:42:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4AA7E3853821 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 77A6A2002D; Thu, 29 Jul 2021 11:42:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1627558936; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rUVNIidHXJKYFn3DyQAesF59GfUcyCeHx2732fhxppU=; b=Oy4mLE8Z0F8W+Ye4DSgub4D+UuZNd68FZeudUs6kJ4XhQGfaLqPTr/TmuF+iNsE1yM7Hgm /3bOjLkdpDuKsa9mK3dZola++TQD9uuutjA/935g4+vII/zAensQHwoAiTJfj9ZJUAzTEZ FUOokVqjQkZYCx0M6ZoXH1AJ3VaSZK0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1627558936; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rUVNIidHXJKYFn3DyQAesF59GfUcyCeHx2732fhxppU=; b=ZWSCnsesU3IuzhCyQURK5/kuIsR5G7ArnjT1CCp9ZZJFLbtlTRXA91ujJa9VVxnPkjW8py yhBz/amPbVHE+BAw== 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 3A0001364E; Thu, 29 Jul 2021 11:42:16 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap1.suse-dmz.suse.de with ESMTPSA id ZGGJChiUAmGAZgAAGKfGzw (envelope-from ); Thu, 29 Jul 2021 11:42:16 +0000 Subject: [master + 11][gdb/build] Disable attribute nonnull From: Tom de Vries To: Tom Tromey References: <20210726211101.ivychvbfgaafxjtz@lug-owl.de> <20210727100354.GB4037238@embecosm.com> <20210727113511.GC4037238@embecosm.com> <6cf80ba9-b010-bb42-c92d-84e4f396813c@suse.de> Message-ID: Date: Thu, 29 Jul 2021 13:42:15 +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: Content-Type: multipart/mixed; boundary="------------80E64F1D7DA06812302CE15B" Content-Language: en-US X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "gdb-patches@sourceware.org" Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" This is a multi-part message in MIME format. --------------80E64F1D7DA06812302CE15B Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit [ was: Re: [gdb/build] Fix Werror=nonnull-compare build breaker with gcc 12 ] On 7/29/21 12:32 AM, Tom de Vries wrote: > [ quote-copied from > https://sourceware.org/pipermail/gdb-patches/2021-July/181173.html. I > didn't get this email in either my inbox or gdb-patches folder. ] > >> Tom> I managed now to reproduce, and wrote a patch along these lines. >> >> Tom> Any comments? >> >> Tom> In particular, any suggestion where to put ignore_nonnull? >> >> Tom> Or, is it perhaps a better idea to have a gdb_assert_nonnull and >> Tom> implement things there? >> > > Thanks for giving your take on this. > >> How about we either drop the nonnull attribute > > That's a possibility indeed. > >> or we use >> -fno-delete-null-pointer-checks? >> > > Unfortunately, that doesn't work (and it took me some hours today to > realize and understand). I've submitted a patch to improved the nonnull > documentation ( > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ), > hopefully it should be clear from there that > -fno-delete-null-pointer-checks doesn't disable the optimization we're > having trouble with. > >> I personally feel that the gcc approach in this area is >> counter-productive, at least for our purposes. My view is that the >> point of this stuff is to help us detect programming errors -- and we're >> uninterested in using non-null-ness as some kind of optimization hint. >> gcc seems to want it both ways, which seems bizarre. > > I think they just implemented two attributes: assume_nonnull and > verify_nonnull as a single attribute nonnull. Then still that could be > workable, but eventually the problem is that you can't switch the two > interpretations on and off in a reasonable manner. > >> But, given that >> this is how the compiler works, IMO we should choose reliability >> whichever way we best can. > > Yes, I hope to get some reasonable feedback, but I'm not holding my > breath. So we could possibly end up with dropping nonnull. > How about this patch (currently building) for master and gdb-11-branch, to have a reliable solution right now for both master and gdb-11-branch, and if and when the discussion at gcc-patches brings further insight, we can improve master? Thanks, - Tom --------------80E64F1D7DA06812302CE15B Content-Type: text/x-patch; charset=UTF-8; name="0001-gdb-build-Disable-attribute-nonnull.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0001-gdb-build-Disable-attribute-nonnull.patch" [gdb/build] Disable attribute nonnull With trunk gcc (12.0) we're running into a -Werror=nonnull-compare build breaker in gdb, which caused a broader review of the usage of the nonnull attribute. The current conclusion is that it's best to disable this. This is explained at length in the gdbsupport/common-defs.h comment. Tested by building with trunk gcc. gdb/ChangeLog: 2021-07-29 Tom de Vries * gdbsupport/common-defs.h (ATTRIBUTE_NONNULL): Disable. --- gdbsupport/common-defs.h | 76 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/gdbsupport/common-defs.h b/gdbsupport/common-defs.h index 5b644010cd9..3ced5371846 100644 --- a/gdbsupport/common-defs.h +++ b/gdbsupport/common-defs.h @@ -110,6 +110,82 @@ #undef ATTRIBUTE_PRINTF #define ATTRIBUTE_PRINTF _GL_ATTRIBUTE_FORMAT_PRINTF_STANDARD +/* This is defined by ansidecl.h, but we disable the attribute. + + Say a developer starts out with: + ... + extern void foo (void *ptr) __atttribute__((nonnull (1))) + void foo (void *ptr) { + } + ... + with the idea in mind to catch: + ... + foo (nullptr); + ... + at compile time with -Werror=nonnull, and then adds: + ... + void foo (void *ptr) { + + gdb_assert (ptr != nullptr); + } + ... + to catch: + ... + foo (variable_with_nullptr_value); + ... + at runtime as well. + + Said developer then verifies that the assert works (using -O0), and commits + the code. + + Some other developer then checks out the code and accidentally writes some + variant of: + ... + foo (variable_with_nullptr_value); + ... + and builds with -O2, and ... the assert doesn't trigger, because it's + optimized away by gcc. + + There's no suppported recipe to prevent the assertion from being optimized + away (other than: build with -O0, or remove the nonnull attribute). Note + that -fno-delete-null-pointer-checks does not help. A patch was submitted + to improve gcc documentation to point his out more clearly ( + https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576218.html ). The + patch also mentions a possible workaround that obfuscated the pointer + using: + ... + void foo (void *ptr) { + + asm ("" : "+r"(ptr)); + gdb_assert (ptr != nullptr); + } + ... + but that still requires the developer to manually add this in all cases + where that's necessary. + + A warning was added to detect the situation: -Wnonnull-compare, which does + help in detecting those cases, but each new gcc release may indicate a new + batch of locations that needs fixing, which means we've added a maintenance + burden. + + We could try to deal with the problem more proactively by introducing a + gdb_assert variant like: + ... + void gdb_assert_non_null (void *ptr) { + asm ("" : "+r"(ptr)); + gdb_assert (ptr != nullptr); + } + void foo (void *ptr) { + gdb_assert_nonnull (ptr); + } + ... + and make it a coding style to use it everywhere, but again, maintenance + burden. + + With all these things considered, for now we go with the solution with the + least maintenance burden: disable the attribute, such that we reliably deal + with it everywhere. */ +#undef ATTRIBUTE_NONNULL +#define ATTRIBUTE_NONNULL(m) + #if GCC_VERSION >= 3004 #define ATTRIBUTE_UNUSED_RESULT __attribute__ ((__warn_unused_result__)) #else --------------80E64F1D7DA06812302CE15B--