On 13/11/2019 13:23, Warner Losh wrote:
On Wed, Nov 13, 2019 at 8:52 AM Pedro Giffuni <p...@freebsd.org
<mailto:p...@freebsd.org>> wrote:
Hi;
On 12/11/2019 23:44, Warner Losh wrote:
On Tue, Nov 12, 2019 at 9:20 PM Kyle Evans <kev...@freebsd.org
<mailto:kev...@freebsd.org>> wrote:
On Tue, Nov 12, 2019, 22:04 Pedro Giffuni <p...@freebsd.org
<mailto:p...@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
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
