Re: libelf-0.175 breaks objtool

Re: libelf-0.175 breaks objtool

[Arnd Bergmann]

On Tue, Apr 7, 2020 at 6:33 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Tue, Apr 07, 2020 at 05:46:23PM +0200, Arnd Bergmann wrote:
> > On Tue, Apr 7, 2020 at 12:31 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Tue, Apr 7, 2020 at 12:16 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > It's also odd that I only see the problem in two specific files:
> > > arch/x86/realmode/rm/trampoline_64.o (in half of the randconfig builds)
> > > and fs/xfs/xfs_trace.o  (in only one configuration so far).
> > >
> > > With this patch I can avoid the first one, which is unconditionally
> > > built with -g (why?):
> > >
> > > --- a/arch/x86/realmode/rm/Makefile
> > > +++ b/arch/x86/realmode/rm/Makefile
> > > @@ -69,7 +69,7 @@ $(obj)/realmode.relocs: $(obj)/realmode.elf FORCE
> > >  # ---------------------------------------------------------------------------
> > >
> > >  KBUILD_CFLAGS  := $(REALMODE_CFLAGS) -D_SETUP -D_WAKEUP \
> > > -                  -I$(srctree)/arch/x86/boot
> > > +                  -I$(srctree)/arch/x86/boot -gz=none
> > >  KBUILD_AFLAGS  := $(KBUILD_CFLAGS) -D__ASSEMBLY__
> > >  KBUILD_CFLAGS  += -fno-asynchronous-unwind-tables
> > >  GCOV_PROFILE := n
> > >
> > > I'll look at the other one tomorrow.
> >
> > I found where -g gets added in both cases, and adding -gz=none
> > seems to address all randconfigs with CONFIG_DEBUG_INFO=n:
> >
> > --- a/fs/xfs/Makefile
> > +++ b/fs/xfs/Makefile
> > @@ -7,7 +7,7 @@
> >  ccflags-y += -I $(srctree)/$(src)              # needed for trace events
> >  ccflags-y += -I $(srctree)/$(src)/libxfs
> >
> > -ccflags-$(CONFIG_XFS_DEBUG) += -g
> > +ccflags-$(CONFIG_XFS_DEBUG) += -g $(call cc-option,-gz=none)
>
> Maybe they shouldn't have -g in the first place?

That is very possible. The -g has been there since xfs was originally merged
back in 2002, and I could not figure out why it was there (unlike the
-DSTATIC=""
and -DDEBUG flags that are set in the same line).

On the other hand, my feeling is that setting -g should not cause problems
with objtool, if CONFIG_DEBUG_INFO is ok.

       Arnd

Re: libelf-0.175 breaks objtool

[Arnd Bergmann]

On Tue, Apr 7, 2020 at 8:44 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Apr 7, 2020 at 6:33 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Tue, Apr 07, 2020 at 05:46:23PM +0200, Arnd Bergmann wrote:

> That is very possible. The -g has been there since xfs was originally merged
> back in 2002, and I could not figure out why it was there (unlike the
> -DSTATIC=""
> and -DDEBUG flags that are set in the same line).
>
> On the other hand, my feeling is that setting -g should not cause problems
> with objtool, if CONFIG_DEBUG_INFO is ok.

Nevermind, I now see the same problem in lots of files in a configuration
setting these options:

CONFIG_DEBUG_INFO=y
CONFIG_DEBUG_INFO_REDUCED=y
# CONFIG_DEBUG_INFO_SPLIT is not set
CONFIG_DEBUG_INFO_DWARF4=y
CONFIG_DEBUG_INFO_BTF=y

Not sure which of these are actually relevant, but I had other configurations
setting DEBUG_INFO that are not affected.

       Arnd

Re: libelf-0.175 breaks objtool

[Christoph Hellwig]

On Tue, Apr 07, 2020 at 08:44:11PM +0200, Arnd Bergmann wrote:
> That is very possible. The -g has been there since xfs was originally merged
> back in 2002, and I could not figure out why it was there (unlike the
> -DSTATIC=""
> and -DDEBUG flags that are set in the same line).
> 
> On the other hand, my feeling is that setting -g should not cause problems
> with objtool, if CONFIG_DEBUG_INFO is ok.

I suspect we shouldn't force -g ourselves in xfs.  Care to send a patch?

Re: libelf-0.175 breaks objtool

[Arnd Bergmann]

On Thu, Apr 9, 2020 at 9:41 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Apr 07, 2020 at 08:44:11PM +0200, Arnd Bergmann wrote:
> > That is very possible. The -g has been there since xfs was originally merged
> > back in 2002, and I could not figure out why it was there (unlike the
> > -DSTATIC=""
> > and -DDEBUG flags that are set in the same line).
> >
> > On the other hand, my feeling is that setting -g should not cause problems
> > with objtool, if CONFIG_DEBUG_INFO is ok.
>
> I suspect we shouldn't force -g ourselves in xfs.  Care to send a patch?

Done.

On a related topic, I noticed how the CONFIG_DEBUG flag used to control
whether functions marked STATIC get inlined or not, but now they are always
marked noinline, apparently in an attempt to get more readable object code
even when not debugging. I also see that during early v2.6, XFS used
'STATIC' almost exclusively, while newly added functions tend to use plain
'static' instead.

Is this something worth revisiting to see if inlining would make a difference
to performance or are you reasonably sure it does not?

       Arnd

Re: libelf-0.175 breaks objtool

[Dave Chinner]

On Thu, Apr 09, 2020 at 10:25:14AM +0200, Arnd Bergmann wrote:
> On Thu, Apr 9, 2020 at 9:41 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, Apr 07, 2020 at 08:44:11PM +0200, Arnd Bergmann wrote:
> > > That is very possible. The -g has been there since xfs was originally merged
> > > back in 2002, and I could not figure out why it was there (unlike the
> > > -DSTATIC=""
> > > and -DDEBUG flags that are set in the same line).
> > >
> > > On the other hand, my feeling is that setting -g should not cause problems
> > > with objtool, if CONFIG_DEBUG_INFO is ok.
> >
> > I suspect we shouldn't force -g ourselves in xfs.  Care to send a patch?
> 
> Done.
> 
> On a related topic, I noticed how the CONFIG_DEBUG flag used to control
> whether functions marked STATIC get inlined or not, but now they are always
> marked noinline, apparently in an attempt to get more readable object code
> even when not debugging. I also see that during early v2.6, XFS used
> 'STATIC' almost exclusively, while newly added functions tend to use plain
> 'static' instead.
> 
> Is this something worth revisiting to see if inlining would make a difference
> to performance or are you reasonably sure it does not?

The "noinline" here is really "noinline_for_stack", but it predates
the noinline_for_stack annotation and never got updated. i.e.
the noinline was added because of the historic stack usage problems
we had with 4k stacks on x86 and 8k stacks on x86-64. We had to
split large functions up to reduce the number of on-stack variables
in them, but that was then being undone by the compiler deciding to
automatically inline single use static functions.

I know the x86-64 stack issues have gone away now we have
CONFIG_VMAP_STACK, but people can still turn that off so we still
have to be careful about stack usage caused by inlining single use
functions...

Hence I'm not sure we can just remove this - we'd need to go back
and investigate what impact it has on stack usage in the writeback
allocation path and other historic problematic code paths to
determine if they should still be annotated or not. They could be
converted with s/STATIC/static noinline_for_stack/ on a case by case
basis, and then the rest could probably just be case converted with
sed...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com