| Message ID | CAK7LNARQggM3aKEPRKJqa4tunFAfmfErMZuS-rrnRv6UB1VpPQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL,1/4] Kbuild updates for v4.21 | expand |
On Fri, Dec 28, 2018 at 8:00 AM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Introduce a new option CONFIG_NO_AUTO_INLINE as well. With this option, > only functions explicitly marked with "inline" will be inlined. This > will allow the function tracer to trace more functions. Ugh. This causes new and bogus warnings, because gcc doesn't see some of the checks. For example, the x86 emulate_vsyscall() function now warns that warning: ‘syscall_nr’ may be used uninitialized in this function because addr_to_vsyscall_nr() isn't inlined any more, so gcc doesn't see that the return value is always smaller than three (and then it doesn't see that we handle all the cases in the switch statement later). So honestly, these "debugging improvements" have a rather nasty side effect. I'm not at all convinced that we should do this. It is *not* worth causing extra development pain with a totally pointless option that changes code generation radically like this. It's going to cause lots of extra churn. This branch already added a couple of extra inline markers just to make code work reasonably. How many tens (or hundreds) got missed just because the build still works, but the lack of inlining means that we generate completely garbage code? I'm going to skip this pull request. We have basically always required a certain level of competence from the compiler, and this basically says that the compiler can do stupid things and we'd have to fix those stupidities by hand. Linus
(+CC Changbin Du, Arnd Bergmann) On Sun, Dec 30, 2018 at 6:01 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Fri, Dec 28, 2018 at 8:00 AM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > > > Introduce a new option CONFIG_NO_AUTO_INLINE as well. With this option, > > only functions explicitly marked with "inline" will be inlined. This > > will allow the function tracer to trace more functions. > > Ugh. This causes new and bogus warnings, because gcc doesn't see some > of the checks. > > For example, the x86 emulate_vsyscall() function now warns that > > warning: ‘syscall_nr’ may be used uninitialized in this function > > because addr_to_vsyscall_nr() isn't inlined any more, so gcc doesn't > see that the return value is always smaller than three (and then it > doesn't see that we handle all the cases in the switch statement > later). > > So honestly, these "debugging improvements" have a rather nasty side > effect. I'm not at all convinced that we should do this. It is *not* > worth causing extra development pain with a totally pointless option > that changes code generation radically like this. It's going to cause > lots of extra churn. Sorry for late reply. I know this series was rejected, but I felt guilty about being completely silent. So, here is just my two cents. For addr_to_vsyscall_nr(), we can fix it by marking it as 'inline' (or __always_inline). IMHO, it is not so bad to tell the compiler what we want. In most cases, it is just a matter of 'inline' marker, although I admit this process is kind of painful... > This branch already added a couple of extra inline markers just to > make code work reasonably. How many tens (or hundreds) got missed just > because the build still works, but the lack of inlining means that we > generate completely garbage code? I do not have the exact number, but my impression was "not so many". Changbin and Arnd might have better insight. They were trying to eliminate potential problems beforehand. For example, 7e17916b 412dd15c 08813e0e > I'm going to skip this pull request. > > We have basically always required a certain level of competence from > the compiler, and this basically says that the compiler can do stupid > things and we'd have to fix those stupidities by hand. > > Linus
On Mon, Jan 07, 2019 at 01:32:56PM +0900, Masahiro Yamada wrote: > (+CC Changbin Du, Arnd Bergmann) > > > [...] > > This branch already added a couple of extra inline markers just to > > make code work reasonably. How many tens (or hundreds) got missed just > > because the build still works, but the lack of inlining means that we > > generate completely garbage code? > > > I do not have the exact number, but > my impression was "not so many". > > Changbin and Arnd might have better insight. > They were trying to eliminate potential problems beforehand. > > For example, > 7e17916b > 412dd15c > 08813e0e > Yes, the case is not so many. At the beginning, I also thought this could require incredible change. But after trying, just found it is not so bad. I have a couple of patches to fix the remaing warnings which adds 'inline' or '_init' markers for related functions. I promise that all warnings get fixed with new version. And since I only have coressponding setup to fully test x86 and ARM platform, I think we could make this new option depend on x86 or arm temparaly so untested platforms will not complain anymore. Linus, is this good for you? Thank you for revewing. > > > > > I'm going to skip this pull request. > > > > We have basically always required a certain level of competence from > > the compiler, and this basically says that the compiler can do stupid > > things and we'd have to fix those stupidities by hand. > > > > Linus > > > > -- > Best Regards > Masahiro Yamada
On Tue, Jan 8, 2019 at 1:43 AM Changbin Du <changbin.du@gmail.com> wrote: > > On Mon, Jan 07, 2019 at 01:32:56PM +0900, Masahiro Yamada wrote: > > (+CC Changbin Du, Arnd Bergmann) > > > > > > > [...] > > > This branch already added a couple of extra inline markers just to > > > make code work reasonably. How many tens (or hundreds) got missed just > > > because the build still works, but the lack of inlining means that we > > > generate completely garbage code? > > > > > > I do not have the exact number, but > > my impression was "not so many". > > > > Changbin and Arnd might have better insight. > > They were trying to eliminate potential problems beforehand. > > > > For example, > > 7e17916b > > 412dd15c > > 08813e0e > > > Yes, the case is not so many. At the beginning, I also thought this could > require incredible change. But after trying, just found it is not so bad. I have > a couple of patches to fix the remaing warnings which adds 'inline' or '_init' > markers for related functions. > > I promise that all warnings get fixed with new version. And since I only have > coressponding setup to fully test x86 and ARM platform, I think we could make > this new option depend on x86 or arm temparaly so untested platforms will > not complain anymore. > > Linus, is this good for you? Thank you for revewing. I actually never followed up on this in November, as I was busy trying to get more of the y2038 work done. In my randconfig tree, I have a number patches that fix individual drivers after your patches so I can get back to a warning-free randconfig build overall, but I haven't had a chance to separate out everything that is needed here from other randconfig fixes (e.g. gcc-9 and clang stuff). I tried to get most changes into a single patch that only marks functions inline, see diffstat below. If we want to go ahead with CONFIG_NO_AUTO_INLINE, I can try to get that into a shape that can be submitted with a proper changelog etc. Arnd commit 6985383a091f4be1a2695b0d20108998237651fa Author: Arnd Bergmann <arnd@arndb.de> Date: Sat Nov 3 22:13:15 2018 +0100 make more functions inline Signed-off-by: Arnd Bergmann <arnd@arndb.de> arch/x86/entry/vsyscall/vsyscall_64.c | 2 +- arch/x86/kernel/cpu/microcode/intel.c | 2 +- arch/x86/kernel/ftrace.c | 2 +- arch/x86/kernel/hw_breakpoint.c | 6 +++--- arch/x86/kernel/quirks.c | 2 +- drivers/clocksource/timer-nps.c | 6 +++--- drivers/dma/stm32-mdma.c | 20 ++++++++++---------- drivers/gpio/gpio-aspeed.c | 2 +- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 2 +- drivers/infiniband/hw/mlx5/cq.c | 6 +++--- drivers/media/cec/cec-pin.c | 55 +++++++++++++++++++++++++++++++------------------------ drivers/mmc/host/sdhci-pci-core.c | 6 +++--- drivers/net/ethernet/broadcom/cnic.c | 18 +++++++++++++----- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++-- drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 16 ++++++++++++---- drivers/net/wireless/ti/wlcore/sdio.c | 2 +- drivers/pci/controller/pci-xgene.c | 2 +- drivers/phy/socionext/phy-uniphier-usb3hs.c | 6 +++--- drivers/scsi/ufs/ufs-qcom.c | 2 +- drivers/soc/qcom/wcnss_ctrl.c | 2 +- drivers/staging/axis-fifo/axis-fifo.c | 4 ++-- drivers/staging/comedi/drivers/pcl816.c | 2 +- drivers/staging/comedi/drivers/pcl818.c | 2 +- drivers/thermal/tegra/soctherm.c | 2 +- drivers/thermal/tegra/tegra-bpmp-thermal.c | 4 ++-- drivers/tty/serial/sh-sci.c | 4 ++-- drivers/video/fbdev/i740fb.c | 2 +- net/vmw_vsock/vmci_transport.c | 2 +- 28 files changed, 105 insertions(+), 81 deletions(-)
Hi Linus, Here are Kbuild updates for v4.21 part 2. The following changes since commit ccda4af0f4b92f7b4c308d3acc262f4a7e3affad: Linux 4.20-rc2 (2018-11-11 17:12:31 -0600) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git tags/kbuild-v4.21-2 for you to fetch changes up to 5cd6239506cb97fb00c5992c1adaf6f9f788c5f2: kernel hacking: support building kernel with -Og optimization level (2018-11-19 23:32:54 +0900) ---------------------------------------------------------------- Kbuild updates for v4.21 (part 2) Debugging experience improvement by Changbin Du. Support -Og optimization level, which offers a reasonable level of optimization while maintaining a good debugging experience. Introduce a new option CONFIG_NO_AUTO_INLINE as well. With this option, only functions explicitly marked with "inline" will be inlined. This will allow the function tracer to trace more functions. ---------------------------------------------------------------- Changbin Du (5): x86/mm: declare check_la57_support() as inline openrisc: make function cache_loop() inline kernel hacking: add a config option to disable compiler auto-inlining ARM: mm: fix build error in fix_to_virt with -Og optimization level kernel hacking: support building kernel with -Og optimization level Makefile | 11 +++++++++++ arch/arm/mm/mmu.c | 2 +- arch/openrisc/mm/cache.c | 2 +- arch/x86/kernel/head64.c | 2 +- include/linux/compiler-gcc.h | 2 ++ init/Kconfig | 21 +++++++++++++++++++++ kernel/configs/tiny.config | 1 + lib/Kconfig.debug | 17 +++++++++++++++++ 8 files changed, 55 insertions(+), 3 deletions(-) -- Best Regards Masahiro Yamada