NetBSD Problem Report #46557

From www@NetBSD.org  Wed Jun  6 20:11:36 2012
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 0554C63BEE1
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  6 Jun 2012 20:11:36 +0000 (UTC)
Message-Id: <20120606201135.2392863BA27@www.NetBSD.org>
Date: Wed,  6 Jun 2012 20:11:35 +0000 (UTC)
From: rhansen@bbn.com
Reply-To: rhansen@bbn.com
To: gnats-bugs@NetBSD.org
Subject: sys/syslog.h can't be compiled on its own with -D_KERNEL
X-Send-Pr-Version: www-1.0

>Number:         46557
>Category:       kern
>Synopsis:       sys/syslog.h can't be compiled on its own with -D_KERNEL
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Jun 06 20:15:00 +0000 2012
>Closed-Date:    Fri Oct 16 07:08:31 +0000 2015
>Last-Modified:  Fri Oct 16 07:08:31 +0000 2015
>Originator:     Richard Hansen
>Release:        6.0_BETA
>Organization:
>Environment:
NetBSD <host> 6.0_BETA NetBSD 6.0_BETA (GENERIC) <stuff> amd64
>Description:
(snapshot of netbsd-6 from 2012-03-28)

With -D_KERNEL, <sys/syslog.h> fails to compile unless <sys/cdefs.h> and <sys/ansi.h> are included before it.
>How-To-Repeat:
$ cd /usr/src/sys
$ echo '#include <sys/syslog.h>' >test.c
$ gcc -ffreestanding -nostdinc -D_KERNEL -I. -c test.c
In file included from test.c:1:0:
./sys/syslog.h: In function 'log':
./sys/syslog.h:241:34: error: expected declaration specifiers before '__printflike'
./sys/syslog.h:242:30: error: expected declaration specifiers or '...' before '__va_list'
./sys/syslog.h:242:41: error: expected '=', ',', ';', 'asm' or '__attribute__' before '__printflike'
./sys/syslog.h:243:32: error: expected '=', ',', ';', 'asm' or '__attribute__' before '__printflike'
./sys/syslog.h:241:6: error: old-style parameter declarations in prototyped function definition
./sys/syslog.h:241:1: error: parameter name omitted
./sys/syslog.h:241:1: error: parameter name omitted
test.c:1:0: error: expected '{' at end of input
>Fix:
diff --git a/src/sys/sys/syslog.h b/src/sys/sys/syslog.h
index 410ad72..60753c4 100644
--- a/src/sys/sys/syslog.h
+++ b/src/sys/sys/syslog.h
@@ -34,6 +34,10 @@
 #ifndef _SYS_SYSLOG_H_
 #define _SYS_SYSLOG_H_

+#include <sys/cdefs.h> /* for __printflike() */
+#include <sys/featuretest.h>
+#include <sys/ansi.h> /* for __va_list */
+
 #define        _PATH_LOG       "/var/run/log"

 /*
@@ -206,10 +210,6 @@ struct syslog_data {
     .log_mask = 0xff, \
 }

-#include <sys/cdefs.h>
-#include <sys/featuretest.h>
-#include <sys/ansi.h>
-
 __BEGIN_DECLS
 void   closelog(void);
 void   openlog(const char *, int, int);

>Release-Note:

>Audit-Trail:
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/46557: sys/syslog.h can't be compiled on its own with
 -D_KERNEL
Date: Sun, 20 Jul 2014 23:08:16 +0000

 On Wed, Jun 06, 2012 at 08:15:00PM +0000, rhansen@bbn.com wrote:
  > (snapshot of netbsd-6 from 2012-03-28)
  > 
  > With -D_KERNEL, <sys/syslog.h> fails to compile unless <sys/cdefs.h> and <sys/ansi.h> are included before it.
  > >How-To-Repeat:
  > $ cd /usr/src/sys
  > $ echo '#include <sys/syslog.h>' >test.c
  > $ gcc -ffreestanding -nostdinc -D_KERNEL -I. -c test.c

 As per longstanding tradition, you need <sys/types.h> first in kernel
 source files.

 If you aren't compiling kernel code, don't use -D_KERNEL. What are you
 trying to do? You might be looking for -D_KMEMUSER.

 When these constraints are honored, it works for me...

 -- 
 David A. Holland
 dholland@netbsd.org

State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Tue, 08 Sep 2015 08:52:40 +0000
State-Changed-Why:
I asked a question, last year...


From: Richard Hansen <rhansen@bbn.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/46557: sys/syslog.h can't be compiled on its own with
 -D_KERNEL
Date: Tue, 8 Sep 2015 16:05:58 -0400

 Apologies for forgetting about this; comments below.

 On 2014-07-20 19:10, David Holland wrote:
 > On Wed, Jun 06, 2012 at 08:15:00PM +0000, rhansen@bbn.com wrote:
 >> (snapshot of netbsd-6 from 2012-03-28)
 >>=20
 >> With -D_KERNEL, <sys/syslog.h> fails to compile unless <sys/cdefs.h> a=
 nd <sys/ansi.h> are included before it.
 >> >How-To-Repeat:
 >> $ cd /usr/src/sys
 >> $ echo '#include <sys/syslog.h>' >test.c
 >> $ gcc -ffreestanding -nostdinc -D_KERNEL -I. -c test.c
 >=20
 > As per longstanding tradition, you need <sys/types.h> first in kernel
 > source files.

 That's not a very discoverable tradition for coders unfamiliar with the
 kernel codebase.

 It'd be good if there was a document saying that <sys/types.h> must be
 included first in all kernel .c files (and the reasons why).  There
 should be lots of pointers to that document to make the policy easy to
 discover.

 Even better would be to follow C best practices and make sure each
 header #includes the other headers it depends on.  The patch I provided
 does this for sys/syslog.h.

 >=20
 > If you aren't compiling kernel code, don't use -D_KERNEL.

 I was compiling kernel code.

 > What are you trying to do? You might be looking for -D_KMEMUSER.

 If I remember correctly, I had created a new .c file to add some new
 functionality to the kernel for a project I was working on.  I got that
 compiler error when I wasn't magically imbued with the knowledge that I
 needed to #include other stuff before sys/syslog.h.  :)

 >=20
 > When these constraints are honored, it works for me...

 It also works for me when I #include other headers first, but I
 shouldn't need to do that.  Is there any reason to not apply the patch I
 provided?

 If you don't want to apply the patch, perhaps it would be good to add a
 comment to the top of sys/syslog.h that says that <sys/types.h> must be
 #included first (along with the justification for not #including it from
 sys/syslog.h itself).

 Thanks,
 Richard

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/46557: sys/syslog.h can't be compiled on its own with
 -D_KERNEL
Date: Wed, 9 Sep 2015 06:23:45 +0000

 On Tue, Sep 08, 2015 at 09:35:01PM +0000, Richard Hansen wrote:
  >  Apologies for forgetting about this; comments below.

 No worries, I think there's at least one other of yours that *I*'ve
 been doing nothing on. :-/

  >  > As per longstanding tradition, you need <sys/types.h> first in kernel
  >  > source files.
  >  
  >  That's not a very discoverable tradition for coders unfamiliar with the
  >  kernel codebase.

 That's true; however, it's been the convention for 40-odd years, and
 lots of kernel headers don't compile if you don't include <sys/types.h> 
 (or <sys/param.h>) first.

 There are 2111 ordinary header files in the (arbitrarily chosen) amd64
 kernel tree I'm looking at right now. This includes MD headers (for
 amd64 and i386 only), but excludes driver headers, and also the
 compat, rump, and external subtrees as those have different ground
 rules.

 Using a quick hack mockup of the kernel build environment (which may
 be missing stuff(*)) I get:

    plain                                1411/2111 compile (67%)
    with -include .../sys/sys/types.h    1654/2111 compile (78%)
    with -include .../sys/sys/param.h    1717/2111 compile (81%)

 Looking at just the primary headers in sys/:
    plain                                161/266 compile (60%)
    with -include .../sys/sys/types.h    221/266 compile (83%)
    with -include .../sys/sys/param.h    239/266 compile (90%)

 Note that not every .h file is meant to be a standalone interface, and
 I'm likely as not missing something from the build environment (I
 haven't inspected the errors from the failed files) so the total
 numbers are lower across the board than they "ought" to be. However,
 you probably get the idea.

 I agree that expecting <sys/types.h> first is suboptimal. However,
 it's a bigger problem than just one header file; and adding
 <sys/types.h> indiscriminately runs the risk of polluting the user
 namespace in violation of POSIX and ISO C; in particular,
 <sys/types.h> is exactly what must *not* be included in any number of
 standards-defined headers that are only supposed to expose a limited
 set of declarations.

 One of these days someone will get around to reorganizing the kernel
 includes into a safer and more structured form. I've talked about this
 elsewhere, both how to set it up and what getting there entails.
 Unfortunately, any substantive organization requires moving around a
 lot of core files, so it has to wait until we've migrated to a source
 control system that tracks file renames. And that remains problematic.

 (After such a reorg there's a strong separation between kernel
 internal and user-exposed headers, and no user-exposed header should
 be included directly, so it becomes possible to reform this without
 making a huge mess in libc.)

 I'm also not really sure if reforming the historical convention is
 worthwhile. However, given that more than half of the headers
 apparently no longer rely on it anyway, maybe it's time for it to go
 in favor of now-standard practice. I have no strong opinion on this,
 but it's probably necessary to establish a consensus on the mailing
 list before moving ahead.

 (I don't approve of indiscriminate use of <sys/param.h>, which exposes
 all kinds of random rubbish, but that's a different and probably
 uphill battle.)

  >  It'd be good if there was a document saying that <sys/types.h> must be
  >  included first in all kernel .c files (and the reasons why).  There
  >  should be lots of pointers to that document to make the policy easy to
  >  discover.

 I have no idea where to put that document. I think the information
 appears in the style rules; but that isn't a particularly useful
 place. It ought to appear in the section 9 man pages, but it doesn't,
 and I don't see any good place to put it.

 Unfortunately, it's mostly part of the Unix lore. I don't remember
 when I picked it up; it was a long time ago.

 Any suggestions?

  >  > What are you trying to do? You might be looking for -D_KMEMUSER.
  >  
  >  If I remember correctly, I had created a new .c file to add some new
  >  functionality to the kernel for a project I was working on.  I got that
  >  compiler error when I wasn't magically imbued with the knowledge that I
  >  needed to #include other stuff before sys/syslog.h.  :)

 Fair enough... :-/

 -- 
 David A. Holland
 dholland@netbsd.org

State-Changed-From-To: feedback->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Wed, 09 Sep 2015 06:37:18 +0000
State-Changed-Why:
feedback received.

My inclination is to keep this open and mark it suspended until we can
undertake such a cleanup. Is that ok? It would still probably be helpful
to accumulate a consensus on whether it's time to phase out the historical
expectation.


From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org, rhansen@bbn.com
Subject: re: kern/46557: sys/syslog.h can't be compiled on its own with -D_KERNEL
Date: Wed, 09 Sep 2015 16:47:41 +1000

 share/misc/style kind of hints that you should include <sys/types.h>,
 but it could do with this being explicit.

 that said, i wouldn't object to Richard's patch.


 .mrg.

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
	netbsd-bugs@netbsd.org, gnats-admin@netbsd.org, dholland@NetBSD.org, 
	rhansen@bbn.com
Cc: 
Subject: Re: kern/46557 (sys/syslog.h can't be compiled on its own with -D_KERNEL)
Date: Wed, 9 Sep 2015 07:31:00 -0400

 On Sep 9,  6:37am, dholland@NetBSD.org (dholland@NetBSD.org) wrote:
 -- Subject: Re: kern/46557 (sys/syslog.h can't be compiled on its own with -D

 | My inclination is to keep this open and mark it suspended until we can
 | undertake such a cleanup. Is that ok? It would still probably be helpful
 | to accumulate a consensus on whether it's time to phase out the historical
 | expectation.

 I don't think that such undertaking is worth-while since none of
 the kernel headers are documented to be standalone. If you do that
 then you will end up including <sys/systm.h> <sys/types.h>
 <sys/param.h> <sys/ansi.h> <sys/proc.h> from a lot of places and
 this will happen over and over again for each header in the source
 file. Also if you do that you should clean the userland visible
 headers to avoid duplicate inclusions for efficiency. I.e. if you
 add <sys/ansi.h> to <sys/syslog.h> you should remove it from
 <syslog.h>. Things are fine as they are now; if you are compiling
 kernel files it is up to you to figure out and add the include
 dependencies. It is not like if you fix it once, it will stay fixed
 since there is no harness to check that each header is compilable
 standalone (but you will need to write one too).

 christos

From: Richard Hansen <rhansen@bbn.com>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/46557: sys/syslog.h can't be compiled on its own with
 -D_KERNEL
Date: Wed, 9 Sep 2015 18:34:02 -0400

 On 2015-09-09 02:25, David Holland wrote:
 > I agree that expecting <sys/types.h> first is suboptimal. However,
 > it's a bigger problem than just one header file; and adding
 > <sys/types.h> indiscriminately runs the risk of polluting the user
 > namespace in violation of POSIX and ISO C;

 Thank you for the detailed explanation of the surrounding issues, and
 how this isn't a trivial matter.

 > it has to wait until we've migrated to a source control system that
 > tracks file renames.  And that remains problematic.

 :(  The lack of an official Git repository is one of the reasons why I
 haven't contributed as much as I could have.

 On 2015-09-09 02:37, dholland@NetBSD.org wrote:
 > My inclination is to keep this open and mark it suspended until we can
 > undertake such a cleanup.  Is that ok?

 Sounds good to me.

 On 2015-09-09 07:31, Christos Zoulas wrote:
 > I don't think that such undertaking is worth-while

 This undertaking would be a huge time sink with no short-term gain, but
 there is long-term value:  You'd have code that is more
 readable/understandable by potential new contributors.

 > since none of the kernel headers are documented to be standalone.

 Nowadays the expectation in the broader C community is that headers are
 standalone.  Headers that aren't standalone need to be documented as
 such, not the other way around.

 > If you do that then you will end up including <sys/systm.h>
 > <sys/types.h> <sys/param.h> <sys/ansi.h> <sys/proc.h> from a lot of
 > places and this will happen over and over again for each header in
 > the source file.

 I don't see how that's a problem.  Include guards prevent duplicate
 inclusions from being noticed by the compiler.

 > Also if you do that you should clean the userland visible
 > headers to avoid duplicate inclusions for efficiency.

 Would the performance hit be noticeable?  I suspect the overhead of
 processing include guards and skipping lines of input code is orders of
 magnitude less than the work of compiling the resulting preprocessed code.

 > It is not like if you fix it once, it will stay fixed since there is
 > no harness to check that each header is compilable standalone (but
 > you will need to write one too).

 For this reason I have the following personal policy that I try to
 impose on anyone that will listen:

     For every header file, there is a compiled .c file that includes
     the header file *first*.  This is true even if the .c file does
     nothing other than include the header file.

 If this policy is religiously followed, every header file is guaranteed
 to be standalone.

 -Richard

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/46557: sys/syslog.h can't be compiled on its own with
 -D_KERNEL
Date: Sun, 11 Oct 2015 01:21:32 +0000

 On Thu, Sep 10, 2015 at 12:00:01AM +0000, Richard Hansen wrote:
  >  > it has to wait until we've migrated to a source control system that
  >  > tracks file renames.  And that remains problematic.
  >  
  >  :(  The lack of an official Git repository is one of the reasons why I
  >  haven't contributed as much as I could have.

 Git doesn't track renames well enough. :-|

 (among other issues)

  >  > I don't think that such undertaking is worth-while
  >  
  >  This undertaking would be a huge time sink with no short-term gain, but
  >  there is long-term value:  You'd have code that is more
  >  readable/understandable by potential new contributors.

 Right.

  >  > since none of the kernel headers are documented to be standalone.
  >  
  >  Nowadays the expectation in the broader C community is that headers are
  >  standalone.  Headers that aren't standalone need to be documented as
  >  such, not the other way around.

 Right.

  >  > Also if you do that you should clean the userland visible
  >  > headers to avoid duplicate inclusions for efficiency.
  >  
  >  Would the performance hit be noticeable?  I suspect the overhead of
  >  processing include guards and skipping lines of input code is orders of
  >  magnitude less than the work of compiling the resulting preprocessed code.

 Remember NetBSD still supports a wide variety of junkyard hardware :-)
 however, all this is implicit in the big header cleanup anyway.

  >  > It is not like if you fix it once, it will stay fixed since there is
  >  > no harness to check that each header is compilable standalone (but
  >  > you will need to write one too).
  >  
  >  For this reason I have the following personal policy that I try to
  >  impose on anyone that will listen:
  >  
  >      For every header file, there is a compiled .c file that includes
  >      the header file *first*.  This is true even if the .c file does
  >      nothing other than include the header file.
  >  
  >  If this policy is religiously followed, every header file is guaranteed
  >  to be standalone.

 I prefer (for large projects where regressions are likely) to have an
 explicit linting step for this. It's not that hard to write a makefile
 that emits and compiles a .c file for every .h file.

 -- 
 David A. Holland
 dholland@netbsd.org

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/46557: sys/syslog.h can't be compiled on its own with
 -D_KERNEL
Date: Sun, 11 Oct 2015 01:30:36 +0000

 On Sun, Oct 11, 2015 at 01:25:01AM +0000, David Holland wrote:
  >   >  > I don't think that such undertaking is worth-while
  >   >  
  >   >  This undertaking would be a huge time sink with no short-term gain, but
  >   >  there is long-term value:  You'd have code that is more
  >   >  readable/understandable by potential new contributors.
  >  
  >  Right.

 ...and actually the header cleanup is quite valuable because it
 disentangles the kernel and libc headers and institutes a clear
 interface.

 Anyway, adding sys/cdefs.h and sys/featuretest.h is harmless (and
 standard practice is to include sys/cdefs.h in every header that needs
 it) and the whole point of sys/ansi.h is that it's safe to include, so
 I think I'll go ahead and apply the patch.

 -- 
 David A. Holland
 dholland@netbsd.org

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/46557 (sys/syslog.h can't be compiled on its own with
 -D_KERNEL)
Date: Sun, 11 Oct 2015 01:33:56 +0000

 On Wed, Sep 09, 2015 at 11:35:00AM +0000, Christos Zoulas wrote:
  >  Also if you do that you should clean the userland visible
  >  headers to avoid duplicate inclusions for efficiency. I.e. if you
  >  add <sys/ansi.h> to <sys/syslog.h> you should remove it from
  >  <syslog.h>.

 one other point:
 lrwxr-xr-x  1 root  wheel  12 Sep 11  2014 /usr/include/syslog.h -> sys/syslog.h

 there is no <syslog.h> to clean :-)

 -- 
 David A. Holland
 dholland@netbsd.org

From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/46557 CVS commit: src/sys/sys
Date: Thu, 15 Oct 2015 06:15:22 +0000

 Module Name:	src
 Committed By:	dholland
 Date:		Thu Oct 15 06:15:22 UTC 2015

 Modified Files:
 	src/sys/sys: syslog.h

 Log Message:
 Include <sys/cdefs.h>, <sys/featuretest.h>, and <sys/ansi.h>
 unconditionally, not only #ifndef _KERNEL. The kernel declarations
 require cdefs.h and standard practice is to include cdefs.h where it's
 used; they also require sys/ansi.h; and while they don't require
 featuretest.h it's also harmless... and includes should be at the top
 anyhow.

 PR 46557 from Richard Hansen.


 To generate a diff of this commit:
 cvs rdiff -u -r1.37 -r1.38 src/sys/sys/syslog.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Fri, 16 Oct 2015 07:08:31 +0000
State-Changed-Why:
this one's fixed in HEAD now.


>Unformatted:

Home	PR Database Search