Date: Wed, 13 Nov 2019 14:22:59 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Warner Losh <imp@bsdimp.com> Cc: Kyle Evans <kevans@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r354672 - head/lib/libc/secure Message-ID: <25c3bb4a-971f-be14-4192-409ec6dc019b@FreeBSD.org> In-Reply-To: <CANCZdfre3TL36CMx1j1MTtxq9sNf6NRsjE5o3Qx6Up0vd7srUw@mail.gmail.com> References: <201911130300.xAD30WUE099996@repo.freebsd.org> <80479651-f60d-e352-1f40-f01939aff9fd@FreeBSD.org> <CACNAnaGYZwLkEdUj%2BhQHyCRx=V1AW7P5s33i8xuBVsqo=9hKAA@mail.gmail.com> <CANCZdfoSpH7%2BkBdCQS%2BZSoyeESfXMTWt1ZQCMuUAtkyD_mPCgg@mail.gmail.com> <e446f0d2-9703-6776-83fe-4ff02fa8a8ea@FreeBSD.org> <CANCZdfre3TL36CMx1j1MTtxq9sNf6NRsjE5o3Qx6Up0vd7srUw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 13/11/2019 13:23, Warner Losh wrote: > > > On Wed, Nov 13, 2019 at 8:52 AM Pedro Giffuni <pfg@freebsd.org > <mailto:pfg@freebsd.org>> wrote: > > Hi; > > On 12/11/2019 23:44, Warner Losh wrote: >> >> >> On Tue, Nov 12, 2019 at 9:20 PM Kyle Evans <kevans@freebsd.org >> <mailto:kevans@freebsd.org>> wrote: >> >> >> >> On Tue, Nov 12, 2019, 22:04 Pedro Giffuni <pfg@freebsd.org >> <mailto:pfg@freebsd.org>> 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.<something>. 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?25c3bb4a-971f-be14-4192-409ec6dc019b>