| Message ID | 20201001011232.4050282-2-andrew@lunn.ch |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | driver/net/ethernet W=1 by default | expand |
On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote: > > There is a movement to try to make more and more of /drivers W=1 > clean. But it will only stay clean if new warnings are quickly > detected and fixed, ideally by the developer adding the new code. > > To allow subdirectories to sign up to being W=1 clean for a given > definition of W=1, export the current set of additional compile flags > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can > then use: > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930) > > To indicate they want to W=1 warnings as defined on 20200930. > > Additional warnings can be added to the W=1 definition. This will not > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will > start appearing unless W=1 is actually added to the command > line. Developers can then take their time to fix any new W=1 warnings, > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol. I'm not a fan of this approach. Are DATESTAMP configs just going to keep being added? Is it going to complicate isolating the issue from a randconfig build? If we want more things to build warning-free at W=1, then why don't we start moving warnings from W=1 into the default, until this is no W=1 left? That way we're cutting down on the kernel's configuration combinatorial explosion, rather than adding to it? > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > scripts/Makefile.extrawarn | 34 ++++++++++++++++++---------------- > 1 file changed, 18 insertions(+), 16 deletions(-) > > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn > index 95e4cdb94fe9..957dca35ae3e 100644 > --- a/scripts/Makefile.extrawarn > +++ b/scripts/Makefile.extrawarn > @@ -20,24 +20,26 @@ export KBUILD_EXTRA_WARN > # > # W=1 - warnings which may be relevant and do not occur too often > # > -ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) > - > -KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter > -KBUILD_CFLAGS += -Wmissing-declarations > -KBUILD_CFLAGS += -Wmissing-format-attribute > -KBUILD_CFLAGS += -Wmissing-prototypes > -KBUILD_CFLAGS += -Wold-style-definition > -KBUILD_CFLAGS += -Wmissing-include-dirs > -KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable) > -KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable) > -KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned) > -KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation) > +KBUILD_CFLAGS_W1_20200930 += -Wextra -Wunused -Wno-unused-parameter > +KBUILD_CFLAGS_W1_20200930 += -Wmissing-declarations > +KBUILD_CFLAGS_W1_20200930 += -Wmissing-format-attribute > +KBUILD_CFLAGS_W1_20200930 += -Wmissing-prototypes > +KBUILD_CFLAGS_W1_20200930 += -Wold-style-definition > +KBUILD_CFLAGS_W1_20200930 += -Wmissing-include-dirs > +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-but-set-variable) > +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-const-variable) > +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wpacked-not-aligned) > +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wstringop-truncation) > # The following turn off the warnings enabled by -Wextra > -KBUILD_CFLAGS += -Wno-missing-field-initializers > -KBUILD_CFLAGS += -Wno-sign-compare > -KBUILD_CFLAGS += -Wno-type-limits > +KBUILD_CFLAGS_W1_20200930 += -Wno-missing-field-initializers > +KBUILD_CFLAGS_W1_20200930 += -Wno-sign-compare > +KBUILD_CFLAGS_W1_20200930 += -Wno-type-limits > + > +export KBUILD_CFLAGS_W1_20200930 > + > +ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) > > -KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1 > +KBUILD_CPPFLAGS += $(KBUILD_CFLAGS_W1_20200930) -DKBUILD_EXTRA_WARN1 > > else > > -- > 2.28.0 > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201001011232.4050282-2-andrew%40lunn.ch.
On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote: > On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > There is a movement to try to make more and more of /drivers W=1 > > clean. But it will only stay clean if new warnings are quickly > > detected and fixed, ideally by the developer adding the new code. > > > > To allow subdirectories to sign up to being W=1 clean for a given > > definition of W=1, export the current set of additional compile flags > > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can > > then use: > > > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930) > > > > To indicate they want to W=1 warnings as defined on 20200930. > > > > Additional warnings can be added to the W=1 definition. This will not > > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will > > start appearing unless W=1 is actually added to the command > > line. Developers can then take their time to fix any new W=1 warnings, > > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol. > > I'm not a fan of this approach. Are DATESTAMP configs just going to > keep being added? Is it going to complicate isolating the issue from a > randconfig build? If we want more things to build warning-free at > W=1, then why don't we start moving warnings from W=1 into the > default, until this is no W=1 left? That way we're cutting down on > the kernel's configuration combinatorial explosion, rather than adding > to it? Hi Nick I don't see randconfig being an issue. driver/net/ethernet would always be build W=1, by some stable definition of W=1. randconfig would not enable or disable additional warnings. It to make it clear, KBUILD_CFLAGS_W1_20200930 is not a Kconfig option you can select. It is a Makefile constant, a list of warnings which define what W=1 means on that specific day. See patch 1/2. I see a few issues with moving individual warnings from W=1 to the default: One of the comments for v1 of this patchset is that we cannot introduce new warnings in the build. The complete tree needs to clean of a particularly warning, before it can be added to the default list. But that is not how people are cleaning up code, nor how the infrastructure is designed. Those doing the cleanup are not take the first from the list, -Wextra and cleanup up the whole tree for that one warnings. They are rather enabling W=1 on a subdirectory, and cleanup up all warnings on that subdirectory. So using this approach, in order to move a warning from W=1 to the default, we are going to have to get the entire tree W=1 clean, and move them all the warnings are once. People generally don't care about the tree as a whole. They care about their own corner. The idea of fixing one warning thought the whole tree is 'slicing and dicing' the kernel the wrong way. As we have seen with the recent work with W=1, the more natural way to slice/dice the kernel is by subdirectories. I do however understand the fear that we end up with lots of KBUILD_CFLAGS_W1_<DATESTAMP> constants. So i looked at the git history of script/Makefile.extrawarn. These are historically the symbols we would have, if we started this idea 1/1/2018: KBUILD_CFLAGS_W1_20200326 # CLANG only change KBUILD_CFLAGS_W1_20190907 KBUILD_CFLAGS_W1_20190617 # CLANG only change KBUILD_CFLAGS_W1_20190614 # CLANG only change KBUILD_CFLAGS_W1_20190509 KBUILD_CFLAGS_W1_20180919 KBUILD_CFLAGS_W1_20180111 So not too many. Andrew
Hi Andrew,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git e13dbc4f41db7f7b86f17a2efd7fbe19fc5b6d7a
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/b50d78df08d105cf0f0f2a1f4d2225656fd531cc
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
git checkout b50d78df08d105cf0f0f2a1f4d2225656fd531cc
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/firmware/efi/libstub/x86-stub.c:669:15: warning: no previous prototype for 'efi_main' [-Wmissing-prototypes]
669 | unsigned long efi_main(efi_handle_t handle,
| ^~~~~~~~
vim +/efi_main +669 drivers/firmware/efi/libstub/x86-stub.c
291f36325f9f252 arch/x86/boot/compressed/eboot.c Matt Fleming 2011-12-12 663
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 664 /*
8acf63efa1712fa drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 665 * On success, we return the address of startup_32, which has potentially been
8acf63efa1712fa drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 666 * relocated by efi_relocate_kernel.
8acf63efa1712fa drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 667 * On failure, we exit to the firmware via efi_exit instead of returning.
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 668 */
8acf63efa1712fa drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 @669 unsigned long efi_main(efi_handle_t handle,
c3710de5065d63f arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2019-12-24 670 efi_system_table_t *sys_table_arg,
796eb8d26a57f91 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-01-13 671 struct boot_params *boot_params)
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 672 {
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 673 unsigned long bzimage_addr = (unsigned long)startup_32;
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 674 unsigned long buffer_start, buffer_end;
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 675 struct setup_header *hdr = &boot_params->hdr;
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 676 efi_status_t status;
54b52d872680348 arch/x86/boot/compressed/eboot.c Matt Fleming 2014-01-10 677
ccc27ae77494252 drivers/firmware/efi/libstub/x86-stub.c Ard Biesheuvel 2020-04-16 678 efi_system_table = sys_table_arg;
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 679
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 680 /* Check if we were booted by the EFI firmware */
ccc27ae77494252 drivers/firmware/efi/libstub/x86-stub.c Ard Biesheuvel 2020-04-16 681 if (efi_system_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
3b8f44fc0810d51 drivers/firmware/efi/libstub/x86-stub.c Ard Biesheuvel 2020-02-16 682 efi_exit(handle, EFI_INVALID_PARAMETER);
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 683
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 684 /*
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 685 * If the kernel isn't already loaded at a suitable address,
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 686 * relocate it.
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 687 *
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 688 * It must be loaded above LOAD_PHYSICAL_ADDR.
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 689 *
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 690 * The maximum address for 64-bit is 1 << 46 for 4-level paging. This
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 691 * is defined as the macro MAXMEM, but unfortunately that is not a
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 692 * compile-time constant if 5-level paging is configured, so we instead
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 693 * define our own macro for use here.
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 694 *
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 695 * For 32-bit, the maximum address is complicated to figure out, for
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 696 * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 697 * KASLR uses.
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 698 *
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 699 * Also relocate it if image_offset is zero, i.e. the kernel wasn't
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 700 * loaded by LoadImage, but rather by a bootloader that called the
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 701 * handover entry. The reason we must always relocate in this case is
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 702 * to handle the case of systemd-boot booting a unified kernel image,
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 703 * which is a PE executable that contains the bzImage and an initrd as
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 704 * COFF sections. The initrd section is placed after the bzImage
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 705 * without ensuring that there are at least init_size bytes available
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 706 * for the bzImage, and thus the compressed kernel's startup code may
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 707 * overwrite the initrd unless it is moved out of the way.
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 708 */
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 709
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 710 buffer_start = ALIGN(bzimage_addr - image_offset,
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 711 hdr->kernel_alignment);
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 712 buffer_end = buffer_start + hdr->init_size;
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 713
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 714 if ((buffer_start < LOAD_PHYSICAL_ADDR) ||
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 715 (IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE) ||
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 716 (IS_ENABLED(CONFIG_X86_64) && buffer_end > MAXMEM_X86_64_4LEVEL) ||
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 717 (image_offset == 0)) {
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 718 status = efi_relocate_kernel(&bzimage_addr,
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 719 hdr->init_size, hdr->init_size,
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 720 hdr->pref_address,
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 721 hdr->kernel_alignment,
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 722 LOAD_PHYSICAL_ADDR);
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 723 if (status != EFI_SUCCESS) {
36bdd0a78d56831 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-30 724 efi_err("efi_relocate_kernel() failed!\n");
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 725 goto fail;
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 726 }
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 727 /*
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 728 * Now that we've copied the kernel elsewhere, we no longer
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 729 * have a set up block before startup_32(), so reset image_offset
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 730 * to zero in case it was set earlier.
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 731 */
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 732 image_offset = 0;
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 733 }
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 734
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Andrew,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git e13dbc4f41db7f7b86f17a2efd7fbe19fc5b6d7a
config: parisc-randconfig-r016-20200930 (attached as .config)
compiler: hppa64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b50d78df08d105cf0f0f2a1f4d2225656fd531cc
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
git checkout b50d78df08d105cf0f0f2a1f4d2225656fd531cc
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
arch/parisc/boot/compressed/firmware.c: In function 'set_firmware_width_unlocked':
>> arch/parisc/boot/compressed/firmware.c:159:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
159 | int ret;
| ^~~
arch/parisc/boot/compressed/firmware.c: In function 'set_firmware_width':
arch/parisc/boot/compressed/firmware.c:176:16: warning: variable 'flags' set but not used [-Wunused-but-set-variable]
176 | unsigned long flags;
| ^~~~~
arch/parisc/boot/compressed/firmware.c: In function 'pdc_iodc_print':
arch/parisc/boot/compressed/firmware.c:1234:16: warning: variable 'flags' set but not used [-Wunused-but-set-variable]
1234 | unsigned long flags;
| ^~~~~
At top level:
arch/parisc/boot/compressed/firmware.c:121:22: warning: 'f_extend' defined but not used [-Wunused-function]
121 | static unsigned long f_extend(unsigned long address)
| ^~~~~~~~
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote: > > On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > There is a movement to try to make more and more of /drivers W=1 > > > clean. But it will only stay clean if new warnings are quickly > > > detected and fixed, ideally by the developer adding the new code. Nice, I think everyone agrees that this is a good goal. > > > To allow subdirectories to sign up to being W=1 clean for a given > > > definition of W=1, export the current set of additional compile flags > > > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can > > > then use: > > > > > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930) > > > > > > To indicate they want to W=1 warnings as defined on 20200930. > > > > > > Additional warnings can be added to the W=1 definition. This will not > > > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will > > > start appearing unless W=1 is actually added to the command > > > line. Developers can then take their time to fix any new W=1 warnings, > > > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol. > > > > I'm not a fan of this approach. Are DATESTAMP configs just going to > > keep being added? Is it going to complicate isolating the issue from a > > randconfig build? If we want more things to build warning-free at > > W=1, then why don't we start moving warnings from W=1 into the > > default, until this is no W=1 left? That way we're cutting down on > > the kernel's configuration combinatorial explosion, rather than adding > > to it? I'm also a little sceptical about the datestamp. > Hi Nick > > I don't see randconfig being an issue. driver/net/ethernet would > always be build W=1, by some stable definition of W=1. randconfig > would not enable or disable additional warnings. It to make it clear, > KBUILD_CFLAGS_W1_20200930 is not a Kconfig option you can select. It > is a Makefile constant, a list of warnings which define what W=1 means > on that specific day. See patch 1/2. It won't change with the configuration, but it will change with the specific compiler version. When you enable a warning in a particular driver or directory, this may have different effects on one compiler compared to another: clang and gcc sometimes differ in their interpretation of which specific forms of an expression to warn about or not, and any multiplexing options like -Wextra or -Wformat may turn on additional warnings in later releases. > I see a few issues with moving individual warnings from W=1 to the > default: > > One of the comments for v1 of this patchset is that we cannot > introduce new warnings in the build. The complete tree needs to clean > of a particularly warning, before it can be added to the default list. > But that is not how people are cleaning up code, nor how the > infrastructure is designed. Those doing the cleanup are not take the > first from the list, -Wextra and cleanup up the whole tree for that > one warnings. They are rather enabling W=1 on a subdirectory, and > cleanup up all warnings on that subdirectory. So using this approach, > in order to move a warning from W=1 to the default, we are going to > have to get the entire tree W=1 clean, and move them all the warnings > are once. I think the two approaches are orthogonal, and I would like to see both happening as much as possible: - any warning flag in the W=1 set (including many things implied by -Wextra that have or should have their own flags) that only causes a few lines of output should just be enabled by default after we address the warnings - Code with maintainers that care should have a way to enable the entire W=1 set per directory or per file after addressing all the warnings they do see, with new flags only getting added to W=1 when they don't cause regressions. There are more things that we might want to do on top of this: - identify additional warning flags that we be good to add to W=1 - identify warning flags that are better off being turned into errors, like we do with -Werror=strict-prototypes - Fix the warnings in W=2 that show up in common header files, to actually make it realistic to build specific drivers with W=2 and not have interesting issues drowned out in the noise. - ensure that any warning flag we turn *off* in W=1 or by default is turned on again in one of the higher levels > People generally don't care about the tree as a whole. They care about > their own corner. The idea of fixing one warning thought the whole > tree is 'slicing and dicing' the kernel the wrong way. As we have seen > with the recent work with W=1, the more natural way to slice/dice the > kernel is by subdirectories. I do care about the tree as a whole, and I'm particularly interested in having -Wmissing-declarations/-Wmissing-prototypes enabled globally at some point in the future. Arnd
On Fri, Oct 02, 2020 at 02:20:50PM +0200, Arnd Bergmann wrote: > On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <andrew@lunn.ch> wrote: > > On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote: > > > On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > There is a movement to try to make more and more of /drivers W=1 > > > > clean. But it will only stay clean if new warnings are quickly > > > > detected and fixed, ideally by the developer adding the new code. > > Nice, I think everyone agrees that this is a good goal. > > > > > To allow subdirectories to sign up to being W=1 clean for a given > > > > definition of W=1, export the current set of additional compile flags > > > > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can > > > > then use: > > > > > > > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930) > > > > > > > > To indicate they want to W=1 warnings as defined on 20200930. > > > > > > > > Additional warnings can be added to the W=1 definition. This will not > > > > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will > > > > start appearing unless W=1 is actually added to the command > > > > line. Developers can then take their time to fix any new W=1 warnings, > > > > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol. > > > > > > I'm not a fan of this approach. Are DATESTAMP configs just going to > > > keep being added? Is it going to complicate isolating the issue from a > > > randconfig build? If we want more things to build warning-free at > > > W=1, then why don't we start moving warnings from W=1 into the > > > default, until this is no W=1 left? That way we're cutting down on > > > the kernel's configuration combinatorial explosion, rather than adding > > > to it? > > I'm also a little sceptical about the datestamp. Hi Arnd Any suggestions for an alternative? > It won't change with the configuration, but it will change with the > specific compiler version. When you enable a warning in a > particular driver or directory, this may have different effects > on one compiler compared to another: clang and gcc sometimes > differ in their interpretation of which specific forms of an expression > to warn about or not, and any multiplexing options like -Wextra > or -Wformat may turn on additional warnings in later releases. How do we deal with this at the moment? Or will -Wextra and -Wformat never get moved into the default? > I think the two approaches are orthogonal, and I would like to > see both happening as much as possible: > > - any warning flag in the W=1 set (including many things implied > by -Wextra that have or should have their own flags) that > only causes a few lines of output should just be enabled by > default after we address the warnings Is there currently any simple way to get statistics about how many actual warnings there are per warnings flag in W=1? W=1 on the tree as a whole does not look good, but maybe there is some low hanging fruit and we can quickly promote some from W=1 to default? > - Code with maintainers that care should have a way to enable > the entire W=1 set per directory or per file after addressing all > the warnings they do see, with new flags only getting added to > W=1 when they don't cause regressions. Yes, this is what i'm trying to push forward here. I don't particularly care how it happen, so if somebody comes up with a generally acceptable idea, i will go away and implement it. > > People generally don't care about the tree as a whole. They care about > > their own corner. The idea of fixing one warning thought the whole > > tree is 'slicing and dicing' the kernel the wrong way. As we have seen > > with the recent work with W=1, the more natural way to slice/dice the > > kernel is by subdirectories. > > I do care about the tree as a whole, and I'm particularly interested in > having -Wmissing-declarations/-Wmissing-prototypes enabled globally > at some point in the future. I know you care. But you are vastly out numbered by developers who care about their own little corner. Which is why i said 'generally'. We definitely should come at the problem from both directions, but i guess we can make more progress with tools for the large number of developers each in their own corner, than tools for the few who work tree wide. Andrew
On Fri, Oct 2, 2020 at 2:51 PM Andrew Lunn <andrew@lunn.ch> wrote: > On Fri, Oct 02, 2020 at 02:20:50PM +0200, Arnd Bergmann wrote: > > On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote: > > > > > > > > I'm not a fan of this approach. Are DATESTAMP configs just going to > > > > keep being added? Is it going to complicate isolating the issue from a > > > > randconfig build? If we want more things to build warning-free at > > > > W=1, then why don't we start moving warnings from W=1 into the > > > > default, until this is no W=1 left? That way we're cutting down on > > > > the kernel's configuration combinatorial explosion, rather than adding > > > > to it? > > > > I'm also a little sceptical about the datestamp. > > Hi Arnd > > Any suggestions for an alternative? I think we should deal with it the same way we deal with new compiler versions: before a new compiler starts getting widely used, someone has to address the new warnings that show up, or at the minimum they have to get turned off by default until they are addressed. Today, moving a warning flag from W=1 to default requires that there won't be any regressions in the output. The same should apply to adding W=1 warnings if there is a way for drivers to default-enable them. > > It won't change with the configuration, but it will change with the > > specific compiler version. When you enable a warning in a > > particular driver or directory, this may have different effects > > on one compiler compared to another: clang and gcc sometimes > > differ in their interpretation of which specific forms of an expression > > to warn about or not, and any multiplexing options like -Wextra > > or -Wformat may turn on additional warnings in later releases. > > How do we deal with this at the moment? Or will -Wextra and -Wformat > never get moved into the default? I think for Wextra, that would likely stay with W=1, though individual warnings in that set should be enabled by default whenever they make sense. For -Wformat, we probably want the opposite and enable the global option by default but make individual sub-options W=1 or W=2 if there is too much undesired output. > > I think the two approaches are orthogonal, and I would like to > > see both happening as much as possible: > > > > - any warning flag in the W=1 set (including many things implied > > by -Wextra that have or should have their own flags) that > > only causes a few lines of output should just be enabled by > > default after we address the warnings > > Is there currently any simple way to get statistics about how many > actual warnings there are per warnings flag in W=1? W=1 on the tree as > a whole does not look good, but maybe there is some low hanging fruit > and we can quickly promote some from W=1 to default? I have done this a few times in the past, essentially building a few hundred randconfig kernels across multiple architectures and then processing the output in a script. I usually treat a file:line:warning tuple as a single instance and then count how many there are. > > - Code with maintainers that care should have a way to enable > > the entire W=1 set per directory or per file after addressing all > > the warnings they do see, with new flags only getting added to > > W=1 when they don't cause regressions. > > Yes, this is what i'm trying to push forward here. I don't > particularly care how it happen, so if somebody comes up with a > generally acceptable idea, i will go away and implement it. I actually have a set of patches that I started a while ago to move the logic from scripts/Makefile.extrawarn into include/linux/warnings.h, using '_Pragma("GCC diagnostic ...")' with some infrastructure around it, to also allow drivers to set the level as well as individual warnings when needed. I never managed to get that patch series into a state for submission so far, with a few things that need to be addressed first: - any Makefile that changes warning options needs to be converted to use macro syntax - I need to check that the patches don't accidentally disable warnings that are currently enabled (this is harder than checking the reverse) - some specific warnings have problems with this new method for options that control multiple other options. Arnd
On Thu, Oct 1, 2020 at 6:44 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote: > > On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > There is a movement to try to make more and more of /drivers W=1 > > > clean. But it will only stay clean if new warnings are quickly > > > detected and fixed, ideally by the developer adding the new code. > > > > > > To allow subdirectories to sign up to being W=1 clean for a given > > > definition of W=1, export the current set of additional compile flags > > > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can > > > then use: > > > > > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930) > > > > > > To indicate they want to W=1 warnings as defined on 20200930. > > > > > > Additional warnings can be added to the W=1 definition. This will not > > > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will > > > start appearing unless W=1 is actually added to the command > > > line. Developers can then take their time to fix any new W=1 warnings, > > > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol. > > > > I'm not a fan of this approach. Are DATESTAMP configs just going to > > keep being added? Is it going to complicate isolating the issue from a > > randconfig build? If we want more things to build warning-free at > > W=1, then why don't we start moving warnings from W=1 into the > > default, until this is no W=1 left? That way we're cutting down on > > the kernel's configuration combinatorial explosion, rather than adding > > to it? > > Hi Nick > > I don't see randconfig being an issue. driver/net/ethernet would > always be build W=1, by some stable definition of W=1. randconfig > would not enable or disable additional warnings. It to make it clear, > KBUILD_CFLAGS_W1_20200930 is not a Kconfig option you can select. It > is a Makefile constant, a list of warnings which define what W=1 means > on that specific day. See patch 1/2. > > I see a few issues with moving individual warnings from W=1 to the > default: > > One of the comments for v1 of this patchset is that we cannot > introduce new warnings in the build. The complete tree needs to clean > of a particularly warning, before it can be added to the default list. > But that is not how people are cleaning up code, nor how the > infrastructure is designed. Those doing the cleanup are not take the > first from the list, -Wextra and cleanup up the whole tree for that > one warnings. They are rather enabling W=1 on a subdirectory, and > cleanup up all warnings on that subdirectory. So using this approach, > in order to move a warning from W=1 to the default, we are going to > have to get the entire tree W=1 clean, and move them all the warnings > are once. Sorry, to be more specific about my concern; I like the idea of exporting the W=* flags, then selectively applying them via subdir-ccflags-y. I don't like the idea of supporting W=1 as defined at a precise point in time via multiple date specific symbols. If someone adds something to W=1, then they should need to ensure subdirs build warning-free, so I don't think you need to "snapshot" W=1 based on what it looked like on 20200930. > > People generally don't care about the tree as a whole. They care about > their own corner. The idea of fixing one warning thought the whole > tree is 'slicing and dicing' the kernel the wrong way. As we have seen > with the recent work with W=1, the more natural way to slice/dice the > kernel is by subdirectories. I'm not sure I agree with this paragraph. ^ If a warning is not enabled by default implicitly, then someone would need to clean the tree to turn it on. It's very messy to apply it on a child directory, then try to work up. We've done multiple tree wide warning cleanups and it's not too bad. > > I do however understand the fear that we end up with lots of > KBUILD_CFLAGS_W1_<DATESTAMP> constants. So i looked at the git history > of script/Makefile.extrawarn. These are historically the symbols we > would have, if we started this idea 1/1/2018: > > KBUILD_CFLAGS_W1_20200326 # CLANG only change > KBUILD_CFLAGS_W1_20190907 > KBUILD_CFLAGS_W1_20190617 # CLANG only change > KBUILD_CFLAGS_W1_20190614 # CLANG only change > KBUILD_CFLAGS_W1_20190509 > KBUILD_CFLAGS_W1_20180919 > KBUILD_CFLAGS_W1_20180111 > > So not too many. It's a useful visualization. I still would prefer W=1 to get enabled by default if all of the CI systems have flushed out the existing warnings. That way we have one less combination of things to test; not more. -- Thanks, ~Nick Desaulniers
> Sorry, to be more specific about my concern; I like the idea of > exporting the W=* flags, then selectively applying them via > subdir-ccflags-y. I don't like the idea of supporting W=1 as defined > at a precise point in time via multiple date specific symbols. If > someone adds something to W=1, then they should need to ensure subdirs > build warning-free, so I don't think you need to "snapshot" W=1 based > on what it looked like on 20200930. Hi Nick That then contradicts what Masahiro Yamada said to the first version i posted: https://www.spinics.net/lists/netdev/msg685284.html > With this patch series applied, where should we add -Wfoo-bar? > Adding it to W=1 would emit warnings under drivers/net/ since W=1 is > now the default for the net subsystem. The idea with the date stamps was to allow new warnings to be added to W=1 without them immediately causing warnings on normal builds. You are saying that whoever adds a new warning to W=1 needs to cleanup the tree which is already W=1 clean? That might have the side effect that no more warnings are added to W=1 :-( Andrew
On Mon, Oct 5, 2020 at 9:49 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > Sorry, to be more specific about my concern; I like the idea of > > exporting the W=* flags, then selectively applying them via > > subdir-ccflags-y. I don't like the idea of supporting W=1 as defined > > at a precise point in time via multiple date specific symbols. If > > someone adds something to W=1, then they should need to ensure subdirs > > build warning-free, so I don't think you need to "snapshot" W=1 based > > on what it looked like on 20200930. > > That then contradicts what Masahiro Yamada said to the first version i > posted: > > https://www.spinics.net/lists/netdev/msg685284.html > > With this patch series applied, where should we add -Wfoo-bar? > > Adding it to W=1 would emit warnings under drivers/net/ since W=1 is > > now the default for the net subsystem. > > The idea with the date stamps was to allow new warnings to be added to > W=1 without them immediately causing warnings on normal builds. You > are saying that whoever adds a new warning to W=1 needs to cleanup the > tree which is already W=1 clean? That might have the side effect that > no more warnings are added to W=1 :-( It depends a lot on what portion of the kernel gets enabled for W=1. As long as it's only drivers that are actively maintained, and they make up a fairly small portion of all code, it should not be a problem to find someone to fix useful warnings. The only reason to add a flag to W=1 would be that the bugs it reports are important enough to look at the false positives and address those as well. Whoever decided to enable W=1 by default for their subsystem should then also be interested in adding the new warnings. If I wanted to add a new flag to W=1 and this introduces output for allmodconfig, I would start by mailing that output to the respective maintainers. Arnd
> It depends a lot on what portion of the kernel gets enabled for W=1. > > As long as it's only drivers that are actively maintained, and they > make up a fairly small portion of all code, it should not be a problem > to find someone to fix useful warnings. Well, drivers/net/ethernet is around 1.5M LOC. The tree as a whole is just short of 23M LOC. So i guess that is a small portion of all the code. Andrew
On Tue, Oct 6, 2020 at 6:08 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > It depends a lot on what portion of the kernel gets enabled for W=1. > > > > As long as it's only drivers that are actively maintained, and they > > make up a fairly small portion of all code, it should not be a problem > > to find someone to fix useful warnings. > > Well, drivers/net/ethernet is around 1.5M LOC. The tree as a whole is > just short of 23M LOC. So i guess that is a small portion of all the > code. > > Andrew I am not a big fan of KBUILD_CFLAGS_W1_<timestamp> since it is ugly. I'd like to start with adding individual flags like drivers/gpu/drm/i915/Makefile, and see how difficult it would be to maintain it. One drawback of your approach is that you cannot set KBUILD_CFLAGS_W1_20200930 until you eliminate all the warnings in the sub-directory in interest. (i.e. all or nothing approach) At best, you can only work out from 'old -> new' order because KBUILD_CFLAGS_W1_20200326 is a suer-set of KBUILD_CFLAGS_W1_20190907, which is a suer-set of KBUILD_CFLAGS_W1_20190617 ... If you add flags individually, you can start with low-hanging fruits, or ones with higher priority as Arnd mentions about -Wmissing-{declaration,prototypes}. For example, you might be able to set 'subdir-ccflags-y += -Wmissing-declarations' to drivers/net/Makefile, while 'subdir-ccflags-y += -Wunused-but-set-variable' stays in drivers/net/ethernet/Makefile. -- Best Regards Masahiro Yamada
On Fri, Oct 2, 2020 at 9:21 PM Arnd Bergmann <arnd@arndb.de> wrote: > > I do care about the tree as a whole, and I'm particularly interested in > having -Wmissing-declarations/-Wmissing-prototypes enabled globally > at some point in the future. > > Arnd BTW, if possible, please educate me about the difference between -Wmissing-declarations and -Wmissing-prototypes. The GCC manual says as follows: -Wmissing-prototypes (C and Objective-C only) Warn if a global function is defined without a previous prototype declaration. This warning is issued even if the definition itself provides a prototype. Use this option to detect global functions that do not have a matching prototype declaration in a header file. This option is not valid for C++ because all function declarations provide prototypes and a non-matching declaration declares an overload rather than conflict with an earlier declaration. Use -Wmissing-declarations to detect missing declarations in C++. -Wmissing-declarations Warn if a global function is defined without a previous declaration. Do so even if the definition itself provides a prototype. Use this option to detect global functions that are not declared in header files. In C, no warnings are issued for functions with previous non-prototype declarations; use -Wmissing-prototypes to detect missing prototypes. In C++, no warnings are issued for function templates, or for inline functions, or for functions in anonymous namespaces. The difference is still unclear to me... For example, if I add -Wmissing-declarations, I get the following: kernel/sched/core.c:2380:6: warning: no previous declaration for ‘sched_set_stop_task’ [-Wmissing-declarations] 2380 | void sched_set_stop_task(int cpu, struct task_struct *stop) | ^~~~~~~~~~~~~~~~~~~ But, if I add both -Wmissing-declarations and -Wmissing-prototypes, -Wmissing-declarations is superseded by -Wmissing-prototypes. kernel/sched/core.c:2380:6: warning: no previous prototype for ‘sched_set_stop_task’ [-Wmissing-prototypes] 2380 | void sched_set_stop_task(int cpu, struct task_struct *stop) | ^~~~~~~~~~~~~~~~~~~ Do we need to specify both in W=1 ? If yes, what is the difference between them? -- Best Regards Masahiro Yamada
On Mon, Oct 12, 2020 at 3:00 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > BTW, if possible, please educate me about the difference > between -Wmissing-declarations and -Wmissing-prototypes. > ... > Do we need to specify both in W=1 ? > If yes, what is the difference between them? I think they do the same thing in the kernel, as we already set '-Wstrict-prototypes', which requires that there are no declarations without having a prototype first. Adding either one should be sufficient. Arnd
On Sun, Oct 11, 2020 at 3:04 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > On Tue, Oct 6, 2020 at 6:08 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > I am not a big fan of KBUILD_CFLAGS_W1_<timestamp> > since it is ugly. > > I'd like to start with adding individual flags > like drivers/gpu/drm/i915/Makefile, and see > how difficult it would be to maintain it. I don't really like that either, to be honest, mostly because that is somewhat incompatible with my plan to move all the warning flags out the command line and into _Pragma() lines in header files. > One drawback of your approach is that > you cannot set KBUILD_CFLAGS_W1_20200930 > until you eliminate all the warnings in the > sub-directory in interest. > (i.e. all or nothing approach) > > At best, you can only work out from 'old -> new' order > because KBUILD_CFLAGS_W1_20200326 is a suer-set of > KBUILD_CFLAGS_W1_20190907, which is a suer-set of > KBUILD_CFLAGS_W1_20190617 ... > > > > If you add flags individually, you can start with > low-hanging fruits, or ones with higher priority > as Arnd mentions about -Wmissing-{declaration,prototypes}. > > > For example, you might be able to set > 'subdir-ccflags-y += -Wmissing-declarations' > to drivers/net/Makefile, while > 'subdir-ccflags-y += -Wunused-but-set-variable' > stays in drivers/net/ethernet/Makefile. I agree with the goal though. In particular it may be helpful to pick a set of warning flags to always be enabled without also setting -Wextra, which has different meanings depending on compiler version, or between gcc and clang. I wonder how different they really are, and we can probably find out from https://github.com/Barro/compiler-warnings, but I have not checked myself. Arnd
> One drawback of your approach is that > you cannot set KBUILD_CFLAGS_W1_20200930 > until you eliminate all the warnings in the > sub-directory in interest. > (i.e. all or nothing approach) Hi Mashiro That actual works well for my use case. drivers/net/ethernet is W=1 clean. So is drivers/net/phy, drivers/net/mdio. Developers generally clean up a subsystem by adding W=1 to the command line because that is the simple tool they have. And my aim here is to keep those subsystem W=1 clean. I don't care about individual warnings within W=1, because those subsystems are passed that stage already. Andrew
On Sat, Oct 17, 2020 at 02:48:56PM +0200, Arnd Bergmann wrote: > On Fri, Oct 16, 2020 at 4:12 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > One drawback of your approach is that > > > you cannot set KBUILD_CFLAGS_W1_20200930 > > > until you eliminate all the warnings in the > > > sub-directory in interest. > > > (i.e. all or nothing approach) > > > > Hi Mashiro > > > > That actual works well for my use case. drivers/net/ethernet is W=1 > > clean. So is drivers/net/phy, drivers/net/mdio. Developers generally > > clean up a subsystem by adding W=1 to the command line because that is > > the simple tool they have. > > > > And my aim here is to keep those subsystem W=1 clean. I don't care > > about individual warnings within W=1, because those subsystems are > > passed that stage already. > > I tried to get a better grasp of what kind of warnings we are actually talking > about and looked at the x86 allmodconfig W=1 output on today's linux-next. Hi Arnd The work done to cleanup drivers/net/ethernet was mostly done by an Intel team. When built for ARM there are few warnings left, mostly due to missing COMPILE_TEST. I have fixes for that. But this raises the question, can we be a bit more tolerant of warnings for not x86 to start with? 0-day should help us weed out the remaining warnings on other architectures. As for the plan, it looks O.K. to me. I can definitely help with driver/net and net. Andrew
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 95e4cdb94fe9..957dca35ae3e 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -20,24 +20,26 @@ export KBUILD_EXTRA_WARN # # W=1 - warnings which may be relevant and do not occur too often # -ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) - -KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter -KBUILD_CFLAGS += -Wmissing-declarations -KBUILD_CFLAGS += -Wmissing-format-attribute -KBUILD_CFLAGS += -Wmissing-prototypes -KBUILD_CFLAGS += -Wold-style-definition -KBUILD_CFLAGS += -Wmissing-include-dirs -KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable) -KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable) -KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned) -KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation) +KBUILD_CFLAGS_W1_20200930 += -Wextra -Wunused -Wno-unused-parameter +KBUILD_CFLAGS_W1_20200930 += -Wmissing-declarations +KBUILD_CFLAGS_W1_20200930 += -Wmissing-format-attribute +KBUILD_CFLAGS_W1_20200930 += -Wmissing-prototypes +KBUILD_CFLAGS_W1_20200930 += -Wold-style-definition +KBUILD_CFLAGS_W1_20200930 += -Wmissing-include-dirs +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-but-set-variable) +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-const-variable) +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wpacked-not-aligned) +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wstringop-truncation) # The following turn off the warnings enabled by -Wextra -KBUILD_CFLAGS += -Wno-missing-field-initializers -KBUILD_CFLAGS += -Wno-sign-compare -KBUILD_CFLAGS += -Wno-type-limits +KBUILD_CFLAGS_W1_20200930 += -Wno-missing-field-initializers +KBUILD_CFLAGS_W1_20200930 += -Wno-sign-compare +KBUILD_CFLAGS_W1_20200930 += -Wno-type-limits + +export KBUILD_CFLAGS_W1_20200930 + +ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) -KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1 +KBUILD_CPPFLAGS += $(KBUILD_CFLAGS_W1_20200930) -DKBUILD_EXTRA_WARN1 else
There is a movement to try to make more and more of /drivers W=1 clean. But it will only stay clean if new warnings are quickly detected and fixed, ideally by the developer adding the new code. To allow subdirectories to sign up to being W=1 clean for a given definition of W=1, export the current set of additional compile flags using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can then use: subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930) To indicate they want to W=1 warnings as defined on 20200930. Additional warnings can be added to the W=1 definition. This will not affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will start appearing unless W=1 is actually added to the command line. Developers can then take their time to fix any new W=1 warnings, and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- scripts/Makefile.extrawarn | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-)