From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id POiyHJ/RA2E0YAAAWB0awg (envelope-from ) for ; Fri, 30 Jul 2021 06:17:03 -0400 Received: by simark.ca (Postfix, from userid 112) id 56C4B1EA7E; Fri, 30 Jul 2021 06:17:03 -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.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,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 E727F1EA7E for ; Fri, 30 Jul 2021 06:17:00 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6311A393CC19 for ; Fri, 30 Jul 2021 10:17:00 +0000 (GMT) Received: from mail-wm1-x32e.google.com (mail-wm1-x32e.google.com [IPv6:2a00:1450:4864:20::32e]) by sourceware.org (Postfix) with ESMTPS id C65273853828 for ; Fri, 30 Jul 2021 10:16:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C65273853828 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-wm1-x32e.google.com with SMTP id f14-20020a05600c154eb02902519e4abe10so8815474wmg.4 for ; Fri, 30 Jul 2021 03:16:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=YLdzzZqbwQNR+vE4HaE+H3HGqaK0n+HAuKOrGtd/t0A=; b=AIYyWykxHkxuTMaoGhyXyDP+OPlNbAe0XNQQsKPw6MhKwQYHSxH4bkL9Mg4GIla/2L SGIXF49EsO2F0vXm3bQZOUUbPqa7fJTaFpv4Bll43k2e6quK8kgObet1UD0r/6dqg5kK l2FpL1/uIkAL6oVJj2MYZ60NM63KbuPwk8aVJl/xELmXL2qVtnpCbabcS4KG8kbBoWgE KG5+aGrdPYOEnGLY8av/cPU1YIWViQ0Savlry9+9yBRTYhEfgpSVzxZPTkoK6gEWXqN7 Dbi1+muX9J88SIyoW+DvbFH+BDyVOFmdDHXX0rLXgkwNnYKuDM41QaScojLQl5nboB+w 5rlg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=YLdzzZqbwQNR+vE4HaE+H3HGqaK0n+HAuKOrGtd/t0A=; b=MjbGgD+j29lrmVz2WsXajlfhhSNHUkI2vya5TAmYBm3vEofPYpFk1mURWcbCC/HmZQ cwXRIpK0JOraf2IF9SU7Qej0H4AFR1ap33PbKDOCNafH1J1RL60POFyr/RaqQ3cj3QYf uAELxoBaStFhLRNaNKqxEWpfA7MTen2edF2xpHmvHpYsGgElw/KS6/Oz9OWn3b0sqU4I gnZ/AUaqsKdae9vH+GDZEgpr9Rwtjqc1ow6o8qEKVj5cziBjrVQJQ9BYBRtzXu/pujIn eZHQ/QVMrEMwpLHllPWCtnYFG6rgl/EWDg95L5yKOkznpXhWdi9NpJVCHOIzEsss+bgn LFvQ== X-Gm-Message-State: AOAM532FjeycPljoTuHZ75KMXV8jjLLnyW3bi+wK3hbwz2VFhTZpAPtH lrh/wp9DZ3C63QNmH/a9IOmWwg== X-Google-Smtp-Source: ABdhPJxOkjT6opVPwwyX/p80CMZODSmZkIlnIG+jhipC8brty4K5y45WJ+SgTEu19IcG5Cmv5HAbZw== X-Received: by 2002:a1c:f613:: with SMTP id w19mr2307757wmc.136.1627640207459; Fri, 30 Jul 2021 03:16:47 -0700 (PDT) Received: from localhost (host86-161-16-194.range86-161.btcentralplus.com. [86.161.16.194]) by smtp.gmail.com with ESMTPSA id l41sm1250147wmp.23.2021.07.30.03.16.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 30 Jul 2021 03:16:46 -0700 (PDT) Date: Fri, 30 Jul 2021 11:16:45 +0100 From: Andrew Burgess To: Tom de Vries Subject: Re: [master + 11][gdb/build] Disable attribute nonnull Message-ID: <20210730101645.GB8980@embecosm.com> References: <20210726211101.ivychvbfgaafxjtz@lug-owl.de> <20210727100354.GB4037238@embecosm.com> <20210727113511.GC4037238@embecosm.com> <6cf80ba9-b010-bb42-c92d-84e4f396813c@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 11:12:50 up 2 days, 16:19, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] 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: Tom Tromey , "gdb-patches@sourceware.org" Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" * Tom de Vries [2021-07-29 13:42:15 +0200]: > [ 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 > > [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) I have no problem with this patch going in. I did wonder if something like this would be better: #define ATTRIBUTE_NONNULL(m) __do_not_use_attribute_nonull__ We'd then have to remove all uses of ATTRIBUTE_NONNULL. But maybe this would cause problems if any other sub-projects (readline/gnulib?) used ATTRIBUTE_NONNULL, we don't want to have to edit those source files. So your patch is probably preferred. Thanks, Andrew