From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id C37483858409 for ; Tue, 31 Jan 2023 08:42:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C37483858409 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675154579; h=from:from:reply-to:reply-to:subject:subject: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=7R6ExV3+yyfeypKsNoz04lQW5ujfpv4Q/kVl0dUTZm8=; b=TIbzB+d1UC7If97ATFnB29+Mv8U/PtVhIkiwFy6n4D7J7wCseh6yRThjqfIac5ySngWyUB JKytdgeSVU00+n2qn/gon/Sj3hfMGNdTlK1O9KynEpKfXSmAI51TyC21N6dwqoaFX84YFx +tN5n/6REsVh78J2eBsH4cBxvEgtKoc= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-96-JplYVESPNr6QknwvpS3PTQ-1; Tue, 31 Jan 2023 03:42:58 -0500 X-MC-Unique: JplYVESPNr6QknwvpS3PTQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C250F85A5B1; Tue, 31 Jan 2023 08:42:57 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.223]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 80E87112132C; Tue, 31 Jan 2023 08:42:57 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 30V8gt1n1036181 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Tue, 31 Jan 2023 09:42:55 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 30V8gs2N1036180; Tue, 31 Jan 2023 09:42:54 +0100 Date: Tue, 31 Jan 2023 09:42:54 +0100 From: Jakub Jelinek To: Richard Biener Cc: Patrick Palka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] don't declare header-defined functions both static and inline Message-ID: Reply-To: Jakub Jelinek References: <20230131033707.2597685-1-ppalka@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, Jan 31, 2023 at 08:05:15AM +0100, Richard Biener via Gcc-patches wrote: > On Tue, Jan 31, 2023 at 4:39 AM Patrick Palka via Gcc-patches > wrote: > > > > Many functions defined in our headers are declared 'static inline' which > > is a vestige from when GCC's implementation language was C. But in C++ > > the inline keyword is more than just a compiler hint, and is sufficient > > to give the function the intended semantics. In fact declaring a > > (namespace-scope) function both static and inline is a pessimization > > since static effectively disables the intended definition merging > > behavior that inline provides, and is also a source of (harmless) ODR > > violations when a static inline function gets called from a non-static > > inline one (such as tree_operand_length being called from > > tree_operand_check). > > > > This patch mechanically fixes the vast majority of occurrences of this > > anti-pattern throughout the compiler's headers via the command line > > > > echo gcc/*.h gcc/*/*.h | xargs sed -i 's/^static inline/inline/g' > > > > Besides fixing the ODR violations, this speeds up stage1 cc1plus by > > about 2% and reduces the size of its text segment by 1.5MB. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, would this be OK to > > push now or wait for stage1? > > Speeding up and reducing the size of cc1plus improves on continued > regression in this area. So I'd vote +1 to do this now, let's see what others > think. I lean towards +1 too, but 1) we should make sure we don't do that for installed headers with the exception of headers meant for GCC plugins; in quick skimming I don't see ginclude/ headers among the changed ones, but perhaps doing all languages make install, then applying the patch, make and make install again into another tree and compare all the headers in those tree with the exception of paths with /plugin/include/ in it? 2) we should make sure we don't introduce ODR violations through this, if say 2 headers would define different static inline functions with the same name (or even one static inline and another without that). Don't know what would be best to catch that, get from the patch list of changed functions and then search for it in readelf -wi output using some script, or would LTO bootstrap detect that, or libabigail? What I'm worried about is 2 different headers defining the same function perhaps with different arguments/return values or content and that we happen to never include the two headers in the same TU 3) we have also gcc/ada/gcc-interface/*.h with ada.h:#define INLINE static inline gigi.h:static inline unsigned HOST_WIDE_INT gigi.h:static inline bool gigi.h:static inline bool gigi.h:static inline bool gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree I think we can defer that to Ada maintaners but we should tell them about it 4) there are some static inline also in gcc/config/*/*.h (and some in gcc/common/*/*.h - though in that case it is solely in installed header that shouldn't be changed) avr/avr-protos.h:static inline unsigned pru/pru-protos.h:static inline bool rs6000/rs6000-internal.h:static inline bool rs6000/rs6000-protos.h:static inline bool s390/s390-builtins.h:static inline unsigned int s390/s390-builtins.h:static inline unsigned int s390/vecintrin.h:static inline int The last one is an installed header, so I think we shouldn't touch it, the rest should be considered. Jakub