[Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[David Howells]

Should we forcibly include <linux/err.h> from the gcc command line when
compiling all .c files?  Note:

	warthog>git grep -l '#.*include.*<linux/err[.]h>' -- \*.c | wc -l
	1797

vs:

	warthog>git grep -l 'IS_ERR\|ERR_PTR\|PTR_ERR\|ERR_CAST' -- \*.c | wc -l
	4472

So a lot fewer .c files include it than use it, but according to:

	1: If you use a facility then #include the file that defines/declares
	   that facility.  Don't depend on other header files pulling in ones
	   that you use.

as pointed out to me by Stephen, it shouldn't be assumed that it's available.


However, passing errors in pointers is so very common within the kernel, it
might worth be adding linux/err.h to the automatically included stuff.

David

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[Josh Triplett]

On Fri, Jul 25, 2014 at 01:39:39PM +0100, David Howells wrote:
> Should we forcibly include <linux/err.h> from the gcc command line when
> compiling all .c files?  Note:
> 
> 	warthog>git grep -l '#.*include.*<linux/err[.]h>' -- \*.c | wc -l
> 	1797
> 
> vs:
> 
> 	warthog>git grep -l 'IS_ERR\|ERR_PTR\|PTR_ERR\|ERR_CAST' -- \*.c | wc -l
> 	4472
> 
> So a lot fewer .c files include it than use it, but according to:
> 
> 	1: If you use a facility then #include the file that defines/declares
> 	   that facility.  Don't depend on other header files pulling in ones
> 	   that you use.
> 
> as pointed out to me by Stephen, it shouldn't be assumed that it's available.
> 
> 
> However, passing errors in pointers is so very common within the kernel, it
> might worth be adding linux/err.h to the automatically included stuff.

~/src/linux$ git grep -l 'IS_ERR\|ERR_PTR\|PTR_ERR\|ERR_CAST' -- '*.c' | wc -l
4467
~/src/linux$ find -name '*.c' | wc -l
20430

Functionality used by less than a quarter of the source files in the
kernel doesn't make sense to automatically include.  You have a clear
test that shows whether a file uses this functionality, which would
allow an automated patch adding the necessary #include lines.

That does raise an interesting general issue, though: when a change to
the kernel would involve patching several thousand files, and would
potentially produce a huge number of conflicts (and a huge number of
patches if broken out by subsystem) if fed through the normal process,
but can be described by a very short script that requires no manual
intervention, might it make sense to have a clear procedure for saying
"Hey Linus, can you please run this script on top-of-tree and commit the
result?"?

- Josh Triplett

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[H. Peter Anvin]

On 07/25/2014 04:24 PM, Josh Triplett wrote:
>>
>> However, passing errors in pointers is so very common within the kernel, it
>> might worth be adding linux/err.h to the automatically included stuff.
> 
> ~/src/linux$ git grep -l 'IS_ERR\|ERR_PTR\|PTR_ERR\|ERR_CAST' -- '*.c' | wc -l
> 4467
> ~/src/linux$ find -name '*.c' | wc -l
> 20430
> 
> Functionality used by less than a quarter of the source files in the
> kernel doesn't make sense to automatically include.  You have a clear
> test that shows whether a file uses this functionality, which would
> allow an automated patch adding the necessary #include lines.
> 
> That does raise an interesting general issue, though: when a change to
> the kernel would involve patching several thousand files, and would
> potentially produce a huge number of conflicts (and a huge number of
> patches if broken out by subsystem) if fed through the normal process,
> but can be described by a very short script that requires no manual
> intervention, might it make sense to have a clear procedure for saying
> "Hey Linus, can you please run this script on top-of-tree and commit the
> result?"?
> 

We have such a clear procedure.  It involves pre-arranging with Linus
*before the merge window begins* to run the script in question
*immediately before releasing rc1*.

	-hpa

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[Julia Lawall]

On Mon, 28 Jul 2014, H. Peter Anvin wrote:

> On 07/25/2014 04:24 PM, Josh Triplett wrote:
> >>
> >> However, passing errors in pointers is so very common within the kernel, it
> >> might worth be adding linux/err.h to the automatically included stuff.
> > 
> > ~/src/linux$ git grep -l 'IS_ERR\|ERR_PTR\|PTR_ERR\|ERR_CAST' -- '*.c' | wc -l
> > 4467
> > ~/src/linux$ find -name '*.c' | wc -l
> > 20430
> > 
> > Functionality used by less than a quarter of the source files in the
> > kernel doesn't make sense to automatically include.  You have a clear
> > test that shows whether a file uses this functionality, which would
> > allow an automated patch adding the necessary #include lines.
> > 
> > That does raise an interesting general issue, though: when a change to
> > the kernel would involve patching several thousand files, and would
> > potentially produce a huge number of conflicts (and a huge number of
> > patches if broken out by subsystem) if fed through the normal process,
> > but can be described by a very short script that requires no manual
> > intervention, might it make sense to have a clear procedure for saying
> > "Hey Linus, can you please run this script on top-of-tree and commit the
> > result?"?
> > 
> 
> We have such a clear procedure.  It involves pre-arranging with Linus
> *before the merge window begins* to run the script in question
> *immediately before releasing rc1*.

In this case, would it really create many conflicts?  Does the list of 
included files change often.

If there is an optimial position for the include, it could be hard to make 
a perfect script.

julia

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[H. Peter Anvin]

On 07/28/2014 01:16 PM, Julia Lawall wrote:
>>
>> We have such a clear procedure.  It involves pre-arranging with Linus
>> *before the merge window begins* to run the script in question
>> *immediately before releasing rc1*.
> 
> In this case, would it really create many conflicts?  Does the list of 
> included files change often.
> 
> If there is an optimial position for the include, it could be hard to make 
> a perfect script.
> 

In this case it probably doesn't matter much, because there is a key
difference between this and most other patchsets of this type: there is
an order that this can be done which is safe.  Specifically, add the
#include where it needs to go as a patch series, and then remove it from
the build.  If this is merged into a git tree and the resulting build
fails, it will be obvious what needs to happen, and sfr and Linus are
really good about managing that in linux-next and mainline,
respectively, so I don't even think we need to go to this "special
procedure" for this case.

	-hpa

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[Josh Triplett]

On Mon, Jul 28, 2014 at 03:53:02PM -0700, H. Peter Anvin wrote:
> On 07/28/2014 01:16 PM, Julia Lawall wrote:
> >>
> >> We have such a clear procedure.  It involves pre-arranging with Linus
> >> *before the merge window begins* to run the script in question
> >> *immediately before releasing rc1*.
> > 
> > In this case, would it really create many conflicts?  Does the list of 
> > included files change often.

Across thousands of source files?  Often enough to have significant
conflicts during a merge window.  More importantly, new source files
using error pointers get added often enough that a submitted patch would
likely be incomplete (which would not show up as a merge conflict).

> > If there is an optimial position for the include, it could be hard to make 
> > a perfect script.
> > 
> 
> In this case it probably doesn't matter much, because there is a key
> difference between this and most other patchsets of this type: there is
> an order that this can be done which is safe.  Specifically, add the
> #include where it needs to go as a patch series, and then remove it from
> the build.  If this is merged into a git tree and the resulting build
> fails, it will be obvious what needs to happen, and sfr and Linus are
> really good about managing that in linux-next and mainline,
> respectively, so I don't even think we need to go to this "special
> procedure" for this case.

Perhaps, though see above about the handling of new source files.

But this also seems like a good example of when fine-grained commits can
go wrong.  Given mechanical changes to several thousand source files,
does it really make sense to break those changes up into hundreds of
commits, all of which will share the same commit message except for the
prefix at the start of the subject?  Or would it make more sense to just
commit it and submit it as a single change?

(By contrast with, for instance, warning fixes, for which the changes
warrant subsystem-specific review.)

- Josh Triplett

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[Luck, Tony]

> Perhaps, though see above about the handling of new source files.

Sounds like a job for checkpatch.pl to make sure that <linux/err.h>
is included if a new file uses any of  'IS_ERR\|ERR_PTR\|PTR_ERR\|ERR_CAST'
to help stop this spreading to new files.

It might be harder, but presumably still possible, to have it also check if
any use of those macros is added to a file ... but it couldn't tell just by
looking at the patch ... it would have to peek at the original file to see
if there was already a #include <linux/err.h>

Without a check, or some regular scan, this problem is going to recur.

-Tony

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[Hugh Dickins]

On Tue, 29 Jul 2014, Luck, Tony wrote:

> > Perhaps, though see above about the handling of new source files.
> 
> Sounds like a job for checkpatch.pl to make sure that <linux/err.h>
> is included if a new file uses any of  'IS_ERR\|ERR_PTR\|PTR_ERR\|ERR_CAST'
> to help stop this spreading to new files.
> 
> It might be harder, but presumably still possible, to have it also check if
> any use of those macros is added to a file ... but it couldn't tell just by
> looking at the patch ... it would have to peek at the original file to see
> if there was already a #include <linux/err.h>
> 
> Without a check, or some regular scan, this problem is going to recur.

I keep wondering, what _is_ the problem being discussed here?

If someone uses IS_ERR etc without linux/err.h being included,
does the kernel behave in a dangerously unpredictable way?
Or does the build just fail?

So, there's a rule that everybody should include every header
file that's used directly by a particular source file.

Good rule, but somehow I doubt that omitting linux/err.h
is the sole example of that rule being broken.

And our response to that is to get the kbuild system to include
automatically any header files frequently forgotten?

Hugh

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[Hugh Dickins]

On Tue, Jul 29, 2014 at 9:50 AM, Hugh Dickins <hughd@google.com> wrote:
> And our response to that is to get the kbuild system to include
> automatically any header files frequently forgotten?

Sorry, that was an initial suggestion which the discussion quickly
moved away from.  But I still don't understand why the inclusion of
linux/err.h is a matter of such interest.  How  best to make sweeping
changes, sure; but why to bother to do so for linux/err.h remains a
mystery to me.

Hugh

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[Josh Triplett]

On Mon, Jul 28, 2014 at 11:52:28AM -0700, H. Peter Anvin wrote:
> On 07/25/2014 04:24 PM, Josh Triplett wrote:
> >>
> >> However, passing errors in pointers is so very common within the kernel, it
> >> might worth be adding linux/err.h to the automatically included stuff.
> > 
> > ~/src/linux$ git grep -l 'IS_ERR\|ERR_PTR\|PTR_ERR\|ERR_CAST' -- '*.c' | wc -l
> > 4467
> > ~/src/linux$ find -name '*.c' | wc -l
> > 20430
> > 
> > Functionality used by less than a quarter of the source files in the
> > kernel doesn't make sense to automatically include.  You have a clear
> > test that shows whether a file uses this functionality, which would
> > allow an automated patch adding the necessary #include lines.
> > 
> > That does raise an interesting general issue, though: when a change to
> > the kernel would involve patching several thousand files, and would
> > potentially produce a huge number of conflicts (and a huge number of
> > patches if broken out by subsystem) if fed through the normal process,
> > but can be described by a very short script that requires no manual
> > intervention, might it make sense to have a clear procedure for saying
> > "Hey Linus, can you please run this script on top-of-tree and commit the
> > result?"?
> > 
> 
> We have such a clear procedure.  It involves pre-arranging with Linus
> *before the merge window begins* to run the script in question
> *immediately before releasing rc1*.

That sounds sensible.  Is that documented anywhere?

- Josh Triplett

Re: [Ksummit-discuss] Should we force include <linux/err.h> when compiling all .c files?

[H. Peter Anvin]

On 07/29/2014 07:24 AM, Josh Triplett wrote:
>>
>> We have such a clear procedure.  It involves pre-arranging with Linus
>> *before the merge window begins* to run the script in question
>> *immediately before releasing rc1*.
> 
> That sounds sensible.  Is that documented anywhere?
> 

In the brains of a bunch of kernel developers.  This isn't a common
enough of a thing.

However, this is an exceptional procedure, and as I mentioned it isn't
even necessary in this case.

	-hpa