Date: Fri, 29 Jun 2018 16:13:07 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: David Bright <dab@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r335765 - head/sys/sys Message-ID: <20180629151152.H1477@besplex.bde.org> In-Reply-To: <201806281701.w5SH15eP011261@repo.freebsd.org> References: <201806281701.w5SH15eP011261@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 28 Jun 2018, David Bright wrote:
> Log:
> Remove potential identifier conflict in the EV_SET macro.
>
> PR43905 pointed out a problem with the EV_SET macro if the passed
> struct kevent pointer were specified with an expression with side
> effects (e.g., "kevp++"). This was fixed in rS110241, but by using a
> local block that defined an internal variable (named "kevp") to get
> the pointer value once. This worked, but could cause issues if an
> existing variable named "kevp" is in scope. To avoid that issue,
> jilles@ pointed out that "C99 compound literals and designated
> initializers allow doing this cleanly using a macro". This change
> incorporates that suggestion, essentially verbatim from jilles@
> comment on PR43905, except retaining the old definition for pre-C99 or
> non-STDC (e.g., C++) compilers.
This in unnecessarily elaborate and unportable. It is reported to be broken
for gcc. It leaves the non-C99 case as broken as before (not actually very
broken). It has many style bugs.
> Modified: head/sys/sys/event.h
> ==============================================================================
> --- head/sys/sys/event.h Thu Jun 28 15:30:51 2018 (r335764)
> +++ head/sys/sys/event.h Thu Jun 28 17:01:04 2018 (r335765)
> @@ -49,7 +49,26 @@
> #define EVFILT_EMPTY (-13) /* empty send socket buf */
> #define EVFILT_SYSCOUNT 13
>
> +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
Style bug: testing if an identifier is defined before using it in a cpp
expression. Only broken compilers need this. Some compilers have a
-Wundef flag to enable the brokenness. IIRC, this brokenness is turned
back off by default for system headers, but -Wsystem-headers gives full
warnings for system headers too and FreeBSD is supposed to use the latter
so as to inhibit errors in system headers.
<sys/cdefs.h> has the same style bug for this particular identifier.
However, sys/types.h doesn't have this style bug for this identifier.
The style bug is especially not needed for this identifier because all
C90 and later compilers define this identifier, and there aren't many
other compilers. The others are probably old and also don't support a
-Wundef flag to break themselves.
> #define EV_SET(kevp_, a, b, c, d, e, f) do { \
> + *(kevp_) = (struct kevent){ \
Style bug: beginning of 4-column indentations.
> + .ident = (a), \
> + .filter = (b), \
> + ...
> + }; \
Style bug: further 4-column indentations.
> +} while(0)
> +#else /* Pre-C99 or not STDC (e.g., C++) */
> +/* The definition of the local variable kevp could possibly conflict
> + * with a user-defined value passed in parameters a-f.
> + */
It only conflicts because it is name is in the user namespace. It should
be well known that variables in macros must be named beginning with _2_
underscores (or 1 underscore an an upper case letter for an uglier name).
1 underscore suffices for must implementation names (e.g., for function
parameters and struct members) because most names are in an inner score
that can't conflict. The variable is in an inner scope here too. However,
the -Wshadow option asks for warnings about for shadowed variables even
in inner scopes.
> +#define EV_SET(kevp_, a, b, c, d, e, f) do { \
> struct kevent *kevp = (kevp_); \
> (kevp)->ident = (a); \
> (kevp)->filter = (b); \
This has mounds of old style bugs, mostly created by the wrong fix in
r110241. After r110241, there is no problem except -Wshadow warnings
a variable named kenvp. However, kenvp might be a macro expanding to
'syntax error' or 'macro programmer's common bug #2'
Before r110241, there was no local variable; the arg was named kevp
and it was used directly. This had macro programmer's common bug #1
(side effects). Except, EV_SET() is an unsafe macro by the convention
that unsafe macros are spelled in upper case, so it is the caller's
responsibility to avoid side effects.
r110241 changed this to macro programmer's common bug #2 (namespace
error) and added many style bugs. it added underscores to all the
wrong places -- to the end of the arg name instead of to the beginning
of the variable name. This allowed it to not change the uses of the
variable. However, all these uses became style bugs (bogus parentheses).
Even before r110241, this macro didn't have macro programmer's common
bug #0 (missing parentheses around args).
This part of the macro also gices an example of normal style (8-column
indentation), except it doesn't have a tab after #define or a blank line
after the declarations.
Correct fix:
#define EV_SET(kevp, a, b, c, d, e, f) do { \
struct kevent *__kevp = (kevp); \
\
__kevp->ident = (a); \
__kevp->filter = (b); \
...
> @@ -62,6 +81,7 @@
> (kevp)->ext[2] = 0; \
> (kevp)->ext[3] = 0; \
> } while(0)
> +#endif
No ifdefs required.
> struct kevent {
> __uintptr_t ident; /* identifier for this event */
The bugs after r110241 were very small. This system header, like most :-(,
is full of undocumented namespace pollution like this struct member name
'ident'. ident is more likely to be a macro than kevp, since it is more
generic. In practice, kernel code doesn't do much foot-shooting like
#defining either. No other namespace collisions exception ones detected
by -Wshadow are possible, and those are just style bugs.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180629151152.H1477>