Skip site navigation (1)Skip section navigation (2)
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>