svn commit: r354672 - head/lib/libc/secure
Pedro Giffuni
pfg at FreeBSD.org
Wed Nov 13 19:23:02 UTC 2019
On 13/11/2019 13:23, Warner Losh wrote:
>
>
> On Wed, Nov 13, 2019 at 8:52 AM Pedro Giffuni <pfg at freebsd.org
> <mailto:pfg at 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 at freebsd.org
>> <mailto:kevans at freebsd.org>> wrote:
>>
>>
>>
>> On Tue, Nov 12, 2019, 22:04 Pedro Giffuni <pfg at freebsd.org
>> <mailto:pfg at 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
More information about the svn-src-head
mailing list