[GIT PULL] kbuild updates for v4.7-rc1

[GIT PULL] kbuild updates for v4.7-rc1

[Michal Marek]

Hi Linus,

please pull these kbuild changes for v4.7-rc1:
- New option CONFIG_TRIM_UNUSED_KSYMS which does a two-pass build and
  unexports symbols which are not used in the current config [Nicolas
  Pitre]
- Several kbuild rule cleanups [Masahiro Yamada]
- Warning option adjustments for gcov etc [Arnd Bergmann]
- A few more small fixes

Thanks,
Michal

The following changes since commit f55532a0c0b8bb6148f4e07853b876ef73bc69ca:

  Linux 4.6-rc1 (2016-03-26 16:03:24 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mmarek/kbuild.git kbuild

for you to fetch changes up to c9c6837d39311b0cc14cdbe7c18e815ab44aefb1:

  kbuild: move -Wunused-const-variable to W=1 warning level (2016-05-11 13:05:40 +0200)

----------------------------------------------------------------
Arnd Bergmann (6):
      Kbuild: change CC_OPTIMIZE_FOR_SIZE definition
      Kbuild: disable 'maybe-uninitialized' warning for CONFIG_PROFILE_ALL_BRANCHES
      gcov: disable for COMPILE_TEST
      gcov: disable tree-loop-im to reduce stack usage
      gcov: disable -Wmaybe-uninitialized warning
      kbuild: move -Wunused-const-variable to W=1 warning level

Masahiro Yamada (8):
      kbuild: drop FORCE from PHONY targets
      kbuild: specify modules(_install) as PHONY rather than FORCE
      kbuild: mark help target as PHONY
      kbuild: delete unnecessary "@:"
      kbuild: drop redundant "PHONY += FORCE"
      kbuild: rename cmd_cc_i_c to cmd_cpp_i_c
      kbuild: rename cmd_as_s_S to cmd_cpp_s_S
      kbuild: fix if_change and friends to consider argument order

Maxim Zhukov (1):
      scripts: genksyms: fix resource leak

Michal Marek (1):
      kbuild: Get rid of KBUILD_STR

Nicolas Ferre (1):
      kbuild: fix call to adjust_autoksyms.sh when output directory specified

Nicolas Pitre (13):
      kbuild: record needed exported symbols for modules
      export.h: allow for per-symbol configurable EXPORT_SYMBOL()
      fixdep: accept extra dependencies on stdin
      kbuild: de-duplicate fixdep usage
      kbuild: add fine grained build dependencies for exported symbols
      kbuild: create/adjust generated/autoksyms.h
      kbuild: build sample modules along with the rest of the kernel
      kconfig option for TRIM_UNUSED_KSYMS
      kbuild: better abstract vmlinux sequential prerequisites
      kbuild: Fix dependencies for final vmlinux link
      kbuild: adjust ksym_dep_filter for some cmd_* renames
      kbuild: fix ksym_dep_filter when multiple EXPORT_SYMBOL() on the same line
      kbuild: fix adjust_autoksyms.sh for modules that need only one symbol

Robert Jarzmik (1):
      kbuild: forbid kernel directory to contain spaces and colons

 Makefile                                |  66 +++++++++++++++------
 arch/arm/boot/Makefile                  |   1 -
 arch/arm/boot/bootp/Makefile            |   3 +-
 arch/arm/vdso/Makefile                  |   2 +-
 arch/h8300/boot/compressed/Makefile     |   1 -
 arch/ia64/Makefile                      |   6 +-
 arch/m32r/boot/compressed/Makefile      |   1 -
 arch/mn10300/boot/compressed/Makefile   |   1 -
 arch/nios2/boot/compressed/Makefile     |   1 -
 arch/s390/boot/compressed/Makefile      |   1 -
 arch/sh/boot/compressed/Makefile        |   1 -
 arch/sh/boot/romimage/Makefile          |   1 -
 arch/unicore32/boot/Makefile            |   2 +-
 arch/unicore32/boot/compressed/Makefile |   1 -
 arch/x86/boot/compressed/Makefile       |   1 -
 arch/x86/entry/vdso/Makefile            |   4 +-
 arch/x86/purgatory/Makefile             |   2 -
 arch/x86/realmode/rm/Makefile           |   1 -
 include/linux/export.h                  |  33 ++++++++++-
 init/Kconfig                            |  29 +++++++++
 kernel/gcov/Kconfig                     |   1 +
 scripts/Kbuild.include                  |  45 +++++++++++---
 scripts/Makefile.build                  |  44 +++++++-------
 scripts/Makefile.extrawarn              |   1 +
 scripts/Makefile.lib                    |   8 +--
 scripts/adjust_autoksyms.sh             | 101 ++++++++++++++++++++++++++++++++
 scripts/basic/fixdep.c                  |  61 ++++++++++++++-----
 scripts/genksyms/genksyms.c             |   3 +
 tools/build/Makefile.build              |   8 +--
 29 files changed, 335 insertions(+), 95 deletions(-)
 create mode 100755 scripts/adjust_autoksyms.sh

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Linus Torvalds]

On Thu, May 26, 2016 at 1:33 PM, Michal Marek <mmarek@suse.com> wrote:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/mmarek/kbuild.git kbuild

This pull results in new warnings.

I get new "may be uninitialized" warnings now for me allmodconfig
build, and while I didn't look at them all, the one I looked at was
just entirely crap:

   fs/gfs2/dir.c: In function ‘dir_split_leaf.isra.16’:
   fs/gfs2/dir.c:1021:8: warning: ‘leaf_no’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
     error = get_leaf(dip, leaf_no, &obh);
           ^

yeah no, leaf_no is initialized a few lines up by

        error = get_leaf_nr(dip, index, &leaf_no);

and the fact that gcc can't follow the trivial  error handling is not
our fault. It looks *so* trivial that I wonder why.

That said, I don't see exactly what in the pull request causes this.

My reading of the diff seems to say that you are actually adding
*more* cases of -Wno-maybe-uninitialized, not less.

So I suspect it's almost accidental in just how the Kconfig option
CC_OPTIMIZE_FOR_PERFORMANCE happened, which in turn probably just
changes the options for "make allmiodconfig", and it now picks a
non-size-optimized build that always showed those warnings and I just
didn't see them.

Annoying. I've pulled it, but I wish you would look at this.

              Linus

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Arnd Bergmann]

On Thursday, May 26, 2016 10:26:50 PM CEST Linus Torvalds wrote:
> On Thu, May 26, 2016 at 1:33 PM, Michal Marek <mmarek@suse.com> wrote:
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/mmarek/kbuild.git kbuild
> 
> This pull results in new warnings.
> 
> I get new "may be uninitialized" warnings now for me allmodconfig
> build, and while I didn't look at them all, the one I looked at was
> just entirely crap:
> 
>    fs/gfs2/dir.c: In function ‘dir_split_leaf.isra.16’:
>    fs/gfs2/dir.c:1021:8: warning: ‘leaf_no’ may be used uninitialized
> in this function [-Wmaybe-uninitialized]
>      error = get_leaf(dip, leaf_no, &obh);
>            ^
> 
> yeah no, leaf_no is initialized a few lines up by
> 
>         error = get_leaf_nr(dip, index, &leaf_no);
> 
> and the fact that gcc can't follow the trivial  error handling is not
> our fault. It looks *so* trivial that I wonder why.
> 
> That said, I don't see exactly what in the pull request causes this.
>
> My reading of the diff seems to say that you are actually adding
> *more* cases of -Wno-maybe-uninitialized, not less.

I put the explanation for this into the individual patch commit logs.

> So I suspect it's almost accidental in just how the Kconfig option
> CC_OPTIMIZE_FOR_PERFORMANCE happened, which in turn probably just
> changes the options for "make allmiodconfig", and it now picks a
> non-size-optimized build that always showed those warnings and I just
> didn't see them.

Correct, the point of the CC_OPTIMIZE_FOR_PERFORMANCE change is that
we now see the -Wmaybe-uninitialized warnings by default now. I also
submitted patches for all warnings I saw a while ago, but some only
happen with certain compiler versions that I don't regularly test on,
or are architecture specific.

The specific gfs2 warning is one that I fixed before but it came
back after some back-and-forth discussions, and I forgot to post
it again but kept it in my tree of backlog warning fixes, my
mistake. 

I keep running into new valid Wmaybe-uninitialized warnings on
randconfig builds, e.g. https://lkml.org/lkml/2016/5/27/99 today,
and having them enabled in allmodconfig should make it easier for
patch authors to catch them first.

I don't currently get any other -Wmaybe-uninialized warnings using
gcc-6 on ARM, but I get two warnings on x86, in
drivers/net/wireless/wl3501_cs.c and
drivers/net/wireless/intel/iwlwifi/mvm/nvm.c

I'll have a look at those.

	Arnd

8<----
[PATCH] gfs2: avoid maybe-uninitialized warning again

gcc can not always figure out which code is only used in an error
condition an assignment to indirect argument is only done after the
use of IS_ERR() catches errors. In gfs2, this results in a warning
about correct code:

fs/gfs2/dir.c: In function 'get_first_leaf':
fs/gfs2/dir.c:802:9: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]
fs/gfs2/dir.c: In function 'dir_split_leaf':
fs/gfs2/dir.c:1021:8: warning: 'leaf_no' may be used uninitialized in this function [-Wmaybe-uninitialized]

I managed to work around this earlier using the IS_ERR_VALUE(),
but unfortunately that did not catch all configurations.
This new patch reworks the function in question to us
PTR_ERR_OR_ZERO() instead, which makes it more obvious to the
compiler what happens, and should also result in better object
code.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 67893f12e537 ("gfs2: avoid uninitialized variable warning")

diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 4a01f30e9995..271d93905bac 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -783,12 +783,15 @@ static int get_leaf_nr(struct gfs2_inode *dip, u32 index,
 		       u64 *leaf_out)
 {
 	__be64 *hash;
+	int error;
 
 	hash = gfs2_dir_get_hash_table(dip);
-	if (IS_ERR(hash))
-		return PTR_ERR(hash);
-	*leaf_out = be64_to_cpu(*(hash + index));
-	return 0;
+	error = PTR_ERR_OR_ZERO(hash);
+
+	if (!error)
+		*leaf_out = be64_to_cpu(*(hash + index));
+
+	return error;
 }
 
 static int get_first_leaf(struct gfs2_inode *dip, u32 index,
@@ -798,7 +801,7 @@ static int get_first_leaf(struct gfs2_inode *dip, u32 index,
 	int error;
 
 	error = get_leaf_nr(dip, index, &leaf_no);
-	if (!IS_ERR_VALUE(error))
+	if (!error)
 		error = get_leaf(dip, leaf_no, bh_out);
 
 	return error;
@@ -1014,7 +1017,7 @@ static int dir_split_leaf(struct inode *inode, const struct qstr *name)
 
 	index = name->hash >> (32 - dip->i_depth);
 	error = get_leaf_nr(dip, index, &leaf_no);
-	if (IS_ERR_VALUE(error))
+	if (error)
 		return error;
 
 	/*  Get the old leaf block  */

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Linus Torvalds]

On Fri, May 27, 2016 at 5:33 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>
>
> gcc can not always figure out which code is only used in an error
> condition an assignment to indirect argument is only done after the
> use of IS_ERR() catches errors. In gfs2, this results in a warning
> about correct code:

I figured out why gcc warns.

The code is correct, but gcc isn't able to tell that, because when we
return "int" for the error case from get_leaf_nr(), gcc thinks that
that *could* be a zero.

This code:

        if (IS_ERR(hash))
                return PTR_ERR(hash);

is obviously "return non-zero" to a kernel developer (because that's
how our error codes work), but to a compiler that "return PTR_ERR()"
ends up casting a "long" to an "int" return value.

And the compiler thinks that while the "long" was clearly non-zero and
negative, the "int" it cast things to might be zero.

Yes, a very smart compiler might have figured out that the
IS_ERR_VALUE check also means that the value will be non-zero in "int"
as well, but that actually takes value range analysis, not just "we
already ttested it".

And yes, the error goes away if I turn the "int" into "long" into the
affected codepaths.

I'm not entirely happy about your patch, because I think it makes the
code worse. You fix it by effectively making gcc test the resulting
value after the type conversion. I'd really prefer to figure out some
way to let gcc actually understand this error handling model. Because
it's not just the warning, it also generates unnecessary double tests
(first comparing a 64-bit value against the error range, and then
comparing the truncated 32-bit error code against zero).

Making errors be "long" does fix not just the warning but also the
code generation. But we've traditionally used "int" for error returns,
even though the system call path then again turns it into "long" (for
somewhat related reasons, actually - we want to make sure that the
"int" error code is valid in all 64 bits).

So we *could* just start encouraging people to use "long" for error
handling. It would fix warnings and improve the results. But let me
think about this a bit more.

                    Linus

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Linus Torvalds]

On Fri, May 27, 2016 at 10:23 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> This code:
>
>         if (IS_ERR(hash))
>                 return PTR_ERR(hash);
>
> is obviously "return non-zero" to a kernel developer (because that's
> how our error codes work), but to a compiler that "return PTR_ERR()"
> ends up casting a "long" to an "int" return value.

It doesn't help that gfs2 is using the IS_ERR_VALUE() macro on an
"int", whih is bogus to begin with. It happens to *work*, but by the
time you've cast an error pointer to "int", you've lost enough bits
that "IS_ERR_VALUE()" isn't a valid thing to do. You should just check
against zero (possibly against negative).

But fixing that and just checking against a non-zero int doesn't
actually fix the warning.

              Linus

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Arnd Bergmann]

On Friday, May 27, 2016 10:23:00 AM CEST Linus Torvalds wrote:
> On Fri, May 27, 2016 at 5:33 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> >
> > gcc can not always figure out which code is only used in an error
> > condition an assignment to indirect argument is only done after the
> > use of IS_ERR() catches errors. In gfs2, this results in a warning
> > about correct code:
> 
> I figured out why gcc warns.
> 
> The code is correct, but gcc isn't able to tell that, because when we
> return "int" for the error case from get_leaf_nr(), gcc thinks that
> that *could* be a zero.
> 
> This code:
> 
>         if (IS_ERR(hash))
>                 return PTR_ERR(hash);
> 
> is obviously "return non-zero" to a kernel developer (because that's
> how our error codes work), but to a compiler that "return PTR_ERR()"
> ends up casting a "long" to an "int" return value.
> 
> And the compiler thinks that while the "long" was clearly non-zero and
> negative, the "int" it cast things to might be zero.

Exactly, this was my conclusion back in January when I did the 67893f12e537
patch that fixed it for 32-bit architectures: gcc cannot figure out
that 'if (IS_ERR(ptr))' and the '!err' check are equivalent but it
does manage to find 'IS_ERR(ptr)' and 'IS_ERR_VAL(PTR_ERR(ptr))'
are the same, across multiple inlined functions.

> Yes, a very smart compiler might have figured out that the
> IS_ERR_VALUE check also means that the value will be non-zero in "int"
> as well, but that actually takes value range analysis, not just "we
> already ttested it".
> 
> And yes, the error goes away if I turn the "int" into "long" into the
> affected codepaths.
> 
> I'm not entirely happy about your patch, because I think it makes the
> code worse. You fix it by effectively making gcc test the resulting
> value after the type conversion. I'd really prefer to figure out some
> way to let gcc actually understand this error handling model. Because
> it's not just the warning, it also generates unnecessary double tests
> (first comparing a 64-bit value against the error range, and then
> comparing the truncated 32-bit error code against zero).

I see what you mean. On 32-bit ARM, we get the double compare for any
of the versions, while on 64-bit x86, the current version in mainline
is the worst, while the original code (with the warning) and the version
after my second patch still need the second comparison against zero.

> Making errors be "long" does fix not just the warning but also the
> code generation. But we've traditionally used "int" for error returns,
> even though the system call path then again turns it into "long" (for
> somewhat related reasons, actually - we want to make sure that the
> "int" error code is valid in all 64 bits).
> 
> So we *could* just start encouraging people to use "long" for error
> handling. It would fix warnings and improve the results. But let me
> think about this a bit more.
>
On Friday, May 27, 2016 11:28:48 AM CEST Linus Torvalds wrote:
> It doesn't help that gfs2 is using the IS_ERR_VALUE() macro on an
> "int", whih is bogus to begin with. It happens to *work*, but by the
> time you've cast an error pointer to "int", you've lost enough bits
> that "IS_ERR_VALUE()" isn't a valid thing to do. You should just check
> against zero (possibly against negative).
>
> But fixing that and just checking against a non-zero int doesn't
> actually fix the warning.

Right, removing the IS_ERR_VALUE() gets us back to the state before
my original patch, and we get the warning on all architectures, and
we certainly don't want to promote the use of IS_ERR_VALUE() further
because of the problems you mention with unsigned types that are
shorter than 'long'.

In fact, the patch that I have in my private tree that was hiding the
warning for me on x86 is one that removes all instances of IS_ERR_VALUE()
with arguments other than 'unsigned long', see http://pastebin.com/uYa2mkgC
for reference.

	Arnd

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Linus Torvalds]

On Fri, May 27, 2016 at 1:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> In fact, the patch that I have in my private tree that was hiding the
> warning for me on x86 is one that removes all instances of IS_ERR_VALUE()
> with arguments other than 'unsigned long', see http://pastebin.com/uYa2mkgC
> for reference.

Please just send me that patch, we need to do this (and then add a
cast to pointer (and back to unsigned long) in IS_ERR_VALUE() so that
we get warnings for when that macro is mis-used.

I didn't look at the details of your patch, but I did look at several
IS_ERR_VALUE() uses in the standard kernel, and they were basically
all wrong. Even the ones that used it for the rigth reason (vm_brk()
that returns a pointer or an error in an "unsigned long") had actively
screwed up and truncated that (correct) unsigned long value to "int"
before doing the IS_ERR_VALUE(), which made it all wrong again.

And the other users just looked entirely bogus, and were all just
"zero or negative error code" that has nothing to do with
IS_ERR_VALUE(). The code should just check against zero, not use that
macro that was designed for something different.

                Linus

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Arnd Bergmann]

On Friday, May 27, 2016 1:20:29 PM CEST Linus Torvalds wrote:
> On Fri, May 27, 2016 at 1:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > In fact, the patch that I have in my private tree that was hiding the
> > warning for me on x86 is one that removes all instances of IS_ERR_VALUE()
> > with arguments other than 'unsigned long', see http://pastebin.com/uYa2mkgC
> > for reference.
> 
> Please just send me that patch, we need to do this (and then add a
> cast to pointer (and back to unsigned long) in IS_ERR_VALUE() so that
> we get warnings for when that macro is mis-used.

Ok, I've tried to come up with a summary of what happened so far
on this, and sent the patch your way, leaving out the modified
IS_ERR_VALUE() definition for the moment.

I've added the people on Cc whose drivers had the most invasive
changes.

> I didn't look at the details of your patch, but I did look at several
> IS_ERR_VALUE() uses in the standard kernel, and they were basically
> all wrong.

Yes, that matches what we found earlier this year when Andrzej Hajda
did some work on fixing the worst problems he found.

	Arnd

Re: [GIT PULL] kbuild updates for v4.7-rc1

[Al Viro]

On Fri, May 27, 2016 at 01:20:29PM -0700, Linus Torvalds wrote:

> I didn't look at the details of your patch, but I did look at several
> IS_ERR_VALUE() uses in the standard kernel, and they were basically
> all wrong. Even the ones that used it for the rigth reason (vm_brk()
> that returns a pointer or an error in an "unsigned long") had actively
> screwed up and truncated that (correct) unsigned long value to "int"
> before doing the IS_ERR_VALUE(), which made it all wrong again.
> 
> And the other users just looked entirely bogus, and were all just
> "zero or negative error code" that has nothing to do with
> IS_ERR_VALUE(). The code should just check against zero, not use that
> macro that was designed for something different.

Both gfs2 ones and the one in fs/romfs are crap.  AFS one is simply ridiculous -
        result = generic_file_write_iter(iocb, from);
        if (IS_ERR_VALUE(result)) {
                _leave(" = %zd", result);
                return result;
        }

        _leave(" = %zd", result);
        return result;
In binfmt*.c some callers are valid (vm_mmap and vm_brk values; BTW, the
code in binfmt_flat.c checks for vm_mmap returning 0, which should never
happen, AFAICS).  Most of binfmt_flat.c ones are pure cargo-culting.

net/9p/client.c ones are results of serious mistake in design of 9P.L
extensions to 9P - they assume that numeric values of E... are
architecture-independent and send them over the wire.  The uses of
IS_ERR_VALUE there are papering over that.  Badly.

A couple of sound/* ones should be simply "is negative".

netfilter ones are caused by bad calling conventions of
xt_percpu_counter_alloc().

I hadn't looked through drivers/* users, but judging by fs/, net/ and sound/,
I would expect them to be pointless garbage in the best case and red flags
for broken code in the worst.

IMO IS_ERR_VALUE() should be renamed to something less generic and be used
a lot less than it is right now.