[GIT PULL 2/4] Kbuild updates for v4.21 part2

[GIT PULL 2/4] Kbuild updates for v4.21 part2

[Masahiro Yamada]

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

Re: [GIT PULL 2/4] Kbuild updates for v4.21 part2

[Linus Torvalds]

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

Re: [GIT PULL 2/4] Kbuild updates for v4.21 part2

[Masahiro Yamada]

(+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



-- 
Best Regards
Masahiro Yamada

Re: [GIT PULL 2/4] Kbuild updates for v4.21 part2

[Changbin Du]

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

-- 
Cheers,
Changbin Du

Re: [GIT PULL 2/4] Kbuild updates for v4.21 part2

[Arnd Bergmann]

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(-)