From owner-svn-src-head@freebsd.org Wed Nov 13 19:23:02 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 06B1E1B915C; Wed, 13 Nov 2019 19:23:02 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47Cvdy042Mz4Xwr; Wed, 13 Nov 2019 19:23:02 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from [192.168.0.2] (unknown [181.52.72.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: pfg) by smtp.freebsd.org (Postfix) with ESMTPSA id 022EB15F2E; Wed, 13 Nov 2019 19:23:00 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Subject: Re: svn commit: r354672 - head/lib/libc/secure To: Warner Losh Cc: Kyle Evans , src-committers , svn-src-all , svn-src-head References: <201911130300.xAD30WUE099996@repo.freebsd.org> <80479651-f60d-e352-1f40-f01939aff9fd@FreeBSD.org> From: Pedro Giffuni Organization: FreeBSD Message-ID: <25c3bb4a-971f-be14-4192-409ec6dc019b@FreeBSD.org> Date: Wed, 13 Nov 2019 14:22:59 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Nov 2019 19:23:02 -0000 On 13/11/2019 13:23, Warner Losh wrote: > > > On Wed, Nov 13, 2019 at 8:52 AM Pedro Giffuni > wrote: > > Hi; > > On 12/11/2019 23:44, Warner Losh wrote: >> >> >> On Tue, Nov 12, 2019 at 9:20 PM Kyle Evans > > wrote: >> >> >> >> On Tue, Nov 12, 2019, 22:04 Pedro Giffuni > > wrote: >> >> >> On 12/11/2019 22:00, Kyle Evans wrote: >>> Author: kevans >>> Date: Wed Nov 13 03:00:32 2019 >>> New Revision: 354672 >>> URL:https://svnweb.freebsd.org/changeset/base/354672 >>> >>> Log: >>> ssp: rework the logic to use priority=200 on clang builds >>> >>> The preproc logic was added at the last minute to appease GCC 4.2, and >>> kevans@ did clearly not go back and double-check that the logic worked out >>> for clang builds to use the new variant. >>> >>> It turns out that clang defines __GNUC__ == 4. Flip it around and check >>> __clang__ as well, leaving a note to remove it later. >>> >> clang reports itself as GCC 4.2, the priority argument >> was introduced in GCC 4.3. >>> Reported by: cem >>> >>> Modified: >>> head/lib/libc/secure/stack_protector.c >>> >>> Modified: head/lib/libc/secure/stack_protector.c >>> ============================================================================== >>> --- head/lib/libc/secure/stack_protector.c Wed Nov 13 02:22:00 2019 (r354671) >>> +++ head/lib/libc/secure/stack_protector.c Wed Nov 13 03:00:32 2019 (r354672) >>> @@ -47,13 +47,15 @@ __FBSDID("$FreeBSD$"); >>> * they're either not usually statically linked or they simply don't do things >>> * in constructors that would be adversely affected by their positioning with >>> * respect to this initialization. >>> + * >>> + * This conditional should be removed when GCC 4.2 is removed. >>> */ >>> -#if defined(__GNUC__) && __GNUC__ <= 4 >>> -#define _GUARD_SETUP_CTOR_ATTR \ >>> - __attribute__((__constructor__, __used__)); >>> -#else >>> +#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ > 4) >>> #define _GUARD_SETUP_CTOR_ATTR \ >>> __attribute__((__constructor__ (200), __used__)); >>> +#else >>> +#define _GUARD_SETUP_CTOR_ATTR \ >>> + __attribute__((__constructor__, __used__)); >>> #endif >>> >>> extern int __sysctl(const int *name, u_int namelen, void *oldp, >> >> Please fix properly. Assuming clang always supported it, >> something like: >> >> #if __GNUC_PREREQ__(4, 3) || __has_attribute(__constructor__) >> >> should work >> >> Cheers, >> >> >> I considered something of this sort, but searching for >> information on the priority argument in the first place was >> annoying enough that I had too much search-fatigue to figure >> out when GCC actually corrected this, thus assuming that GCC5 >> (which seemed to be an all-around good release if memory >> serves) and later (since I confirmed GCC6) was sufficient. >> >> I'll fix it in the morning (~8 hours) if I receive no further >> objections to address. >> >> >> Soon enough this can be removed entirely... Getting it >> pedantically right in the mean time has little value. We don't >> really support gcc5 at the moment. gcc6 and later have good >> support, but anything new between 4.3 and 6.0 likely is poorly >> tagged... >> > > Well, tracking attributes on GCC versions is not easy but I did > spend a good amount of time getting the attributes right on > cdefs.h and while I lost the battle to get support for older GCC > versions deprecated, getting the attributes properly defined in > the GCC 4.2 vs clang vicinity is particularly important. > > Not really. We only support 4.2.1 + freebsd hacks and then > 6.. Further refining stuff is useless. Some people (Panzura I recall) were actually building FreeBSD with external compilers including GCC 4.2.1 without FreeBSD hacks. I suspect we could build fine with GCC 4.3 and 5.x, although I admit I wouldn't see much sense in it. > Refining 4.3 vs 6.0 buys us nothing and distracts our limited > resources getting correct something we are definitely removing from > the tree in a couple of months. Going back and refining it gives us no > practical benefit. While I don't object to the change, per se, I don't > view it as required given our future plans. > It is not terribly difficult: it is just a matter of getting one number right. > We should scrub cdefs.h. We've needed to for a while... cdefs.h is handy to sort things out in the inprobable case a new compiler arrives to the scene. I fully agree  that it can be cleaned though: feel free to start with the linux intel C compiler support and follow with lint. > I particularly dislike the idea of leaving notes of stuff that can > be removed when an existing compiler is gone. In this case, we can > fix this without adding more lines of code, and that also helps in > case the code is MFCd. > > Now ... if you want to be pedantic: this code doesn't handle the > case for non-GCC based compilers, and it probably could be done > more generic and clean in cdefs.h where it can be reused. But I am > not asking for that ;). > > I guess I disagree here. (...) No problem with disagreeing :). Cheers, Pedro. > The current code is adequate and can be MFC'd.  It's not as perfect as > it could be, but it's not wrong enough to fuss with.... If Kyle wants > to, great, I'm not standing in the way, but I want to send the clear > message that we don't need to get gcc 4.2 era stuff perfect because > such distinctions are currently muddy at best. We won't work with a > stock 4.2 compiler, and we already use 4.2 as a proxy for our current > gcc compiler, not a perfect thing today anyway. Spending time on it > doesn't give good value for the time spent on it, especially if we > spend that time on other things that give better ROI. > > Warner > > Warner