Date: Wed, 13 Nov 2019 12:35:03 -0600 From: Kyle Evans <kevans@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: Pedro Giffuni <pfg@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: <CACNAnaFWq%2B1HoHNsbMh8vUzB6%2BVcANW=1mjiH_RH6c56o0sCqA@mail.gmail.com> 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 Wed, Nov 13, 2019 at 12:23 PM Warner Losh <imp@bsdimp.com> wrote: > > > > On Wed, Nov 13, 2019 at 8:52 AM Pedro Giffuni <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> wrote: >>> >>> >>> >>> On Tue, Nov 12, 2019, 22:04 Pedro Giffuni <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=3D200 on clang builds >>>> >>>> The preproc logic was added at the last minute to appease GCC 4.2, a= nd >>>> kevans@ did clearly not go back and double-check that the logic work= ed out >>>> for clang builds to use the new variant. >>>> >>>> It turns out that clang defines __GNUC__ =3D=3D 4. Flip it around an= d 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 >>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D >>>> --- head/lib/libc/secure/stack_protector.c Wed Nov 13 02:22:00 2019 (r= 354671) >>>> +++ head/lib/libc/secure/stack_protector.c Wed Nov 13 03:00:32 2019 (r= 354672) >>>> @@ -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 position= ing with >>>> * respect to this initialization. >>>> + * >>>> + * This conditional should be removed when GCC 4.2 is removed. >>>> */ >>>> -#if defined(__GNUC__) && __GNUC__ <=3D 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 lik= e: >>>> >>>> #if __GNUC_PREREQ__(4, 3) || __has_attribute(__constructor__) >>>> >>>> should work >>>> >>>> Cheers, >>> >>> >>> I considered something of this sort, but searching for information on t= he 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 as= suming that GCC5 (which seemed to be an all-around good release if memory s= erves) and later (since I confirmed GCC6) was sufficient. >>> >>> I'll fix it in the morning (~8 hours) if I receive no further objection= s to address. >> >> >> Soon enough this can be removed entirely... Getting it pedantically righ= t in the mean time has little value. We don't really support gcc5 at the mo= ment. 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 los= t the battle to get support for older GCC versions deprecated, getting the = attributes properly defined in the GCC 4.2 vs clang vicinity is particularl= y important. > > Not really. We only support 4.2.1 + freebsd hacks and then 6.<something>.= Further refining stuff is useless. Refining 4.3 vs 6.0 buys us nothing and= distracts our limited resources getting correct something we are definitel= y 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 re= moved when an existing compiler is gone. In this case, we can fix this with= out 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 fo= r non-GCC based compilers, and it probably could be done more generic and c= lean 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 sen= d the clear message that we don't need to get gcc 4.2 era stuff perfect bec= ause such distinctions are currently muddy at best. We won't work with a st= ock 4.2 compiler, and we already use 4.2 as a proxy for our current gcc com= piler, not a perfect thing today anyway. Spending time on it doesn't give g= ood value for the time spent on it, especially if we spend that time on oth= er things that give better ROI. > I went ahead and fixed it because it was already agreed that __has_attribute is a better test for clang than defined(__clang__), so I might as well get the version correct since I'm already touching the line and writing a commit message for it. I do still think it should unifdef'd when GCC 4.2 goes away, so the note still reflects my wishes. Given that this is a libc/*.c file, we can handle any of the edge cases for non-clang/newer GCC compilers as they crop up, if they crop up. We don't seem to put much effort (that I've noticed) into making sure that libc can be compiled with other compilers than those noted, but we can always make exception if someone comes forward with a need for it.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaFWq%2B1HoHNsbMh8vUzB6%2BVcANW=1mjiH_RH6c56o0sCqA>