From owner-svn-src-head@freebsd.org Wed Nov 13 18:23:45 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 B79BD1B781D for ; Wed, 13 Nov 2019 18:23:45 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com [IPv6:2607:f8b0:4864:20::72e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47CtKX1kfCz4LBC for ; Wed, 13 Nov 2019 18:23:43 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x72e.google.com with SMTP id 71so2638626qkl.0 for ; Wed, 13 Nov 2019 10:23:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=dm8kL+H5vl0y2b5tQO9chVNHCLs5qUgK5cXtPoyqW58=; b=EXQofeTMDXmUbsPknkwipu+NEPJhqn7ngQ0lteB/dTgzaA9HgVckNtEvBtAtVC8Qds 0tn02uhwAfM4ewP19wiTCnYfOyR0OYlI0G4J+TRsVVL20gyLim4ZL4zVu2BSPxaEC+Ka cmYumMB0QPTRfruhVIkkuywRIpuI/bLUM+DZBSIe9S9/9h6y8qFgRKTE3RuVRNOVOafX Z7m9rvBTZ3qVgh15f+MRAehhVw/okiZJJwXqti5jUNz6ZIuwgn6rVWrp1+5S6T+plGVP KOUpdQO5UWybWa9AQNLrDyvz5Pv+8vDdRGBE3gHM3y+fiHanwmVKSrgRmfh28h363fN0 eO0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=dm8kL+H5vl0y2b5tQO9chVNHCLs5qUgK5cXtPoyqW58=; b=nih7fhYZ+Pp2iONM82w6ZMvXOP0ZuF5O8e1eXiBdDlXVOEIIUYdBfRjMwT+g6xwXUl psa5WXdGuTXhroItN2YN+VjLpfq1E9NCprVQQ2imZ5qmgknqnMenWps/Y8PHjMvqX0gz GR6LtdCtFZRDHB+DQnavWdB+qTGX9P4Pm8ScIcOZz5ByEFX/6g9BuOsbjerW4POHf7dd xsAmp8MY3eoJHvK+PdpIFF+BD0tMXCGBH/5CAl+bxfNf9LFxb93GCwVXoF/X3jCaROfh iW6ToZ0mbS99TW1JLtj052Scm9KsQQGeMbhlDSHJGsUWzI6SeTzSd1r8bmu9zppOJ5Cq Mu8A== X-Gm-Message-State: APjAAAXl97KnktFBagJ8SmZDSG8QwfT58H4GSgvLnt+fN+jaM9ov+qXX /sxFctxkefitF7qju5Taxz+uI5Is/vNwe4n1G2UCgQ== X-Google-Smtp-Source: APXvYqycK4sfYow0nwiaMjjGICTdQ63szDb7HqsvyyzfHN0sPWbbLNPCKOsLlOPuTA/4mh5ow6SpvpK4GNYcIkH4LnM= X-Received: by 2002:a05:620a:200e:: with SMTP id c14mr1677901qka.380.1573669422594; Wed, 13 Nov 2019 10:23:42 -0800 (PST) MIME-Version: 1.0 References: <201911130300.xAD30WUE099996@repo.freebsd.org> <80479651-f60d-e352-1f40-f01939aff9fd@FreeBSD.org> In-Reply-To: From: Warner Losh Date: Wed, 13 Nov 2019 11:23:31 -0700 Message-ID: Subject: Re: svn commit: r354672 - head/lib/libc/secure To: Pedro Giffuni Cc: Kyle Evans , src-committers , svn-src-all , svn-src-head X-Rspamd-Queue-Id: 47CtKX1kfCz4LBC X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=EXQofeTM; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::72e) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-3.73 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; FROM_HAS_DN(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-head@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; URI_COUNT_ODD(1.00)[3]; RCPT_COUNT_FIVE(0.00)[5]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; RCVD_IN_DNSWL_NONE(0.00)[e.2.7.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; R_SPF_NA(0.00)[]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; MIME_TRACE(0.00)[0:+,1:+,2:~]; IP_SCORE(-2.73)[ip: (-9.29), ipnet: 2607:f8b0::/32(-2.33), asn: 15169(-1.99), country: US(-0.05)]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; RCVD_TLS_ALL(0.00)[]; RCVD_COUNT_TWO(0.00)[2] Content-Type: text/plain; charset="UTF-8" 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 18:23:45 -0000 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. 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. We should scrub cdefs.h. We've needed to for a while... > 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. 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