From owner-svn-src-head@freebsd.org Wed Nov 13 15:52:01 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 A13FE1B4445; Wed, 13 Nov 2019 15:52:01 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (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 47CpyT3c5Gz4BNq; Wed, 13 Nov 2019 15:52:01 +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 F1FC61462F; Wed, 13 Nov 2019 15:51:58 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Subject: Re: svn commit: r354672 - head/lib/libc/secure To: Warner Losh , Kyle Evans Cc: 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: Date: Wed, 13 Nov 2019 10:51:58 -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 15:52:01 -0000 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. 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 ;). Pedro.