svn commit: r314862 - head/sys/modules/qlxgbe

[Bruce Evans]

On Wed, 8 Mar 2017, Gleb Smirnoff wrote:

> On Wed, Mar 08, 2017 at 07:47:13PM +1100, Bruce Evans wrote:
> B> > On 08/03/2017 00:17, Gleb Smirnoff wrote:
> B> >> On Tue, Mar 07, 2017 at 03:43:49PM +0000, Andriy Gapon wrote:
> B> >> A> Author: avg
> B> >> A> Date: Tue Mar  7 15:43:49 2017
> B> >> A> New Revision: 314862
> B> >> A> URL: https://svnweb.freebsd.org/changeset/base/314862
> B> >> A>
> B> >> A> Log:
> B> >> A>   qlxgbe: add GCC_MS_EXTENSIONS to CFLAGS to make old base GCC happy
> B> >> A>
> B> >> A>   The module uses unnamed structure and union fields and base GCC in
> B> >> A>   stable/10 doesn't like it.
> B> >> A>   I think that that is a C11 feature, so it is courteous of more modern
> B> >> A>   compilers to not complain about it when compiling in C99 mode.
> B> >>
> B> >> There are a lot of code in kernel, that uses anonymous structs and unions.
> B> >> This feature is enabled globally. Why does this module need special treatment?
> B>
> B> There is not a lot of such code.  There are a lot of ugly macros like
> B> 'define v_rdev v_un.vu_cdev' to avoid having such code since it is
> B> unportable.
>
> Bruce, I am sorry, I can't resist from asking that in a straight unpolite way:
>
> Unportable to what? Back to XX century?

Unportable to all versions of standard C up to and including at least
C11.  Unportable by programmers who don't know what is standard.  I
don't know much about C11, but just looked this up.  It has something
like gnu89 anonymous struct/unions but not ms extensions.  clang takes
more than -fms-extensions to support ms extensions, so the kernel can't
be using any beyond gnu89 ones and -fms-extensions in CFLAGS is part
of another suite of errors.  The kernel uses -std=iso9899:1999 but
userland uses the correct -std=gnu99.  -std=iso9899:1999 should disallow
gnu89 anonymous struct/unions, but it only works for gcc.  There must
be almost no code in the kernel using anonymous struct/unions, not
"not a lot" since the misconfigured -std disallows it for C99 compilers.
   (It is strange that use of anonymous struct/unions didn't grow in the
   1990's).  CFLAGS in FreeBSD is almost readable and portable.  It
   doesn't have -std.  So the standard defaults to gnu89 or gnu99.
   Userland got this wrong for a while by setting -std to c99 when
   it first used -std, but now uses gnu99.)

Documentation for anonymous struct/unions is hard to find.  clang has
nalmost no documentation in FreeBSD, and gnu documentation was last
installed where I can find it easily in FreeBSD-9.  The latter seemed
to have nothing about anonymous struct/unions, but I finally found a
pointer to it in gcc.info when I looked up -fms-extensions.  Gnu
spells "anonymous struct/unions" as unnamed fields and has adequate
documentation under the latter.

For gcc, -fs-extensions is documented to do bad things like turning
off the warning for "implicit int", so it is not the default.  It
enablesenables the following extensions to gcc anonymous unnamed fields:
- anonymous struct/unions can have a tag
- the tag on anonymous struct/unions is actually useful (it can be used
   in following definitions)
- somthing related with a typedef name instead of a tag name.

Contrary to the log message for r278913, clang doesn't do the right thing
here without any extra options.  Rather the reverse.  clang fails to
detect an error.  gcc detects the error, and r278913 prevents gcc
detecting the error.

I couldn't find any flags to make clang support ms extensions for
unnamed fileds (it accepts -fms-extensions but still gives warnings
and errors for unnamed fields).  But the kernel apparently only uses
old gnu unnamed fields.  The kernel is compiled with the wrong flags
-std=iso9899:1999.  This should turn off all extensions of C99.  It
turns off old gnu unnamed fields for gcc but not for clang.  Userland
is compiled with the correct flags -std=gnu99.  r278913 adds
-fms-extensions instead of fixing -std.

Example:

X struct bar2 { int i; };
X 
X struct foo {
X 	struct bar { int i; };
X 	struct bar2;
X 	struct bar b1;
X 	struct bar2 b2;
X } f;

clang with no flags, clang -std=c99, clang -std=gnu99 and clang -std=c11
compile this with 2 warnings (that the 2 anoymous structs don't declare
anything).  Adding -f-ms-extensions changes the 2 anonymous structs
to declare ms extensions but still warns about them, and gives a
redefinition error for i in struct bar (says it redefines i in struct
bar2).

gcc-4.2.1 does everything as correctly as possible (it cannot support c11).
Without -fms-extensions, it gives the same warnings as clang.  With
-fms-extensions, it just works.

In this example, the use of ms extensions doesn't do much.  It just allows
declaring b1 without repeating the definition of an anonymous struct bar.
This would be useful if struct bar were much larger.

The other fields are to demonstrate access names and inadertently expose
bugs in clang.  'i' in the first struct bar can be accessed as f.i.  The
other i's reqire more verbose access names.  Actually, the first in the
struct and the 'i' in it can't be named, since 'struct bar2;' is anonymous
so the only possible name for the 'i' in it is f.i, but that conflicts with
f.i for the first struct bar.  clang considers this to be an error.  But
I think gcc is correct in allowing it.  The first struct bar2 can work as
padding, and this might be useful (like anonymost bit-fields for padding).

b1 demonstrates the use of the ms extension.  There is no problem with
its access names, since both b1 and b1.i are named and b1 in b1.i
prevents conflicts, but f.b1.i is not as conveniently named as f.i.

b2 demonstrates normal use of struct declared in outer scope, and the
anonymous use of struct bar was supposed to demonstrate that the outer
declaration works for anonymous fields too.  It might work if there
is no name conflict (remove the first anonymous field for that).  Then
we can reference the 'i' in it with the short access name f.i.  Otherwise,
it just doesn't work for clang, and gives not very useful behaviour of
only padding or bugs for gcc (f.i could be any of the anonymous i's;
if it is always the first one, the the behaviour is unmbiguous but
using it is an obfuscation).

Second example:

X struct foo {
X 	struct { int i; };
X 	struct { int j; };
X } f;

This just simple old gnu and not so old C11 features in a simple way.
This should require -std=gnuN (N >= 89) or std=c11 to compile.  It can
also be compiled by std=cN (N >= 89) plus -fms-extensions.  gcc with
plain -std=cN (89 <= N <= 99) attempts to be a C99 compiler and rejects
it.  clang with plain -std=cN (89 <= N <= 99) doesn't attempt to be a
C99 compiler and accepts it.  clang with -std=cN (89 <= N <= 99) plus
-pedantic gives the most useful messages.  The messages are still only
warnings, but give the reason for rejection (that anonymous struct are
a new (C11) feature).

Bruce