From owner-svn-src-all@freebsd.org Thu Mar 9 11:51:21 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B5868D049E2; Thu, 9 Mar 2017 11:51:21 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 6379C357; Thu, 9 Mar 2017 11:51:20 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id DD40410833F; Thu, 9 Mar 2017 22:51:18 +1100 (AEDT) Date: Thu, 9 Mar 2017 22:51:17 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Gleb Smirnoff cc: Bruce Evans , Andriy Gapon , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314862 - head/sys/modules/qlxgbe In-Reply-To: <20170308215056.GP1044@FreeBSD.org> Message-ID: <20170309203335.M1817@besplex.bde.org> References: <201703071543.v27FhnoL024242@repo.freebsd.org> <20170307221733.GN1044@FreeBSD.org> <70fcdcf4-cfa5-2382-ea60-55ac1a91e06b@FreeBSD.org> <20170308193709.Q2738@besplex.bde.org> <20170308215056.GP1044@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=VbSHBBh9 c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=ltDKUUI1AAAA:8 a=cp5bWA6jezjpOfhMFaMA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 a=zPa3363zVR52QbuIqfxu:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Mar 2017 11:51:21 -0000 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