Re: [RFC 1/2] tracing/Makefile: fix handling redefinition of CC_FLAGS_FTRACE

[Vasily Gorbik <gor@linux.ibm.com>]

On Wed, Sep 05, 2018 at 10:10:59PM -0400, Steven Rostedt wrote:
> On Wed,  5 Sep 2018 15:35:59 -0700
> Paulo Zanoni <paulo.r.zanoni@intel.com> wrote:
> 
> > As a Kernel developer, I make heavy use of "make targz-pkg" in order
> > to locally compile and remotely install my development Kernels. The
> > nice feature I rely on is that after a normal "make", "make targz-pkg"
> > only generates the tarball without having to recompile everything.
> > 
> > That was true until commit f28bc3c32c05 ("tracing: Handle
> > CC_FLAGS_FTRACE more accurately"). After it, running "make targz-pkg"
> > after "make" will recompile the whole Kernel tree, making my
> > development workflow much slower.
> > 
> > The Kernel is choosing to recompile everything because it claims the
> > command line has changed. A diff of the .cmd files show a repeated
> > -mfentry in one of the files. It seems that something on make
> > targz-pkg triggers the double -mfentry but not the double -pg.
> 
> It's probably because the addition of -mfentry isn't protected behind a
> ifndef block (like you added in this patch).

scripts/package/Makefile:
tar%pkg: FORCE
჻·······$(MAKE) KBUILD_SRC=
჻·······$(CONFIG_SHELL) $(srctree)/scripts/package/buildtar $@

scripts/package/buildtar:
if grep -q '^CONFIG_MODULES=y' "${KCONFIG_CONFIG}"; then
჻·······make ARCH="${ARCH}" O="${objtree}" KBUILD_SRC= INSTALL_MOD_PATH="${tmpdir}" modules_install

"make targz-pkg" target calls "make modules_install" and environment is
already populated with exported variables, CC_FLAGS_FTRACE being one
of them. In other words top Makefile is called recursively in
sub-make. I assume, to make it work in general exported variables should be
always recalculated to avoid side effects.

In this particular case:
ifndef CC_FLAGS_FTRACE
CC_FLAGS_FTRACE := -pg
endif
is bad, because we don't know if CC_FLAGS_FTRACE is defined in platform
specific arch/$(SRCARCH)/Makefile (included earlier), or coming from parent
make process as exported variable.

In my view one simple way to fix this particular case would be to move:
ifdef CONFIG_FUNCTION_TRACER
        CC_FLAGS_FTRACE := -pg
endif
before:
include arch/$(SRCARCH)/Makefile
where platforms set custom CC_FLAGS_FTRACE.

Or alternatively change the name of the variable that platforms have to set in
arch/$(SRCARCH)/Makefile (so that it does not conflict with exported variable).

> > 
> > So this patch attempts to deal with that problem by handling -mfentry
> > and others just like we handle -pg: don't append them if
> > CC_FLAGS_FTRACE is already defined. I'm not a Makefile expert and so I
> > can't claim this won't break anything else, hopefully if this patch
> > gets applied it will have a Reviewed-by tag from some actual Makefile
> > expert. I can claim this patch fixes the specific regression I
> > reported.
> > 
> > Fixes: commit f28bc3c32c05 ("tracing: Handle CC_FLAGS_FTRACE more
> >  accurately")
> > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > Cc: Michal Marek <michal.lkml@markovi.net>
> > Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: linux-kbuild@vger.kernel.org
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  Makefile | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 19948e556941..2c8df481b4f1 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -755,28 +755,30 @@ KBUILD_CFLAGS 	+= $(call cc-option, -femit-struct-debug-baseonly) \
> >  endif
> >  
> >  ifdef CONFIG_FUNCTION_TRACER
> > -ifndef CC_FLAGS_FTRACE
> > -CC_FLAGS_FTRACE := -pg
> > -endif
> > +cc_flags_ftrace_ := -pg
> 
> I have no problem with this patch, but why the extra underscore at the
> end of cc_flags_ftrace_, and not just cc_flags_ftrace?
> 
> -- Steve
> 
> >  ifdef CONFIG_FTRACE_MCOUNT_RECORD
> >    # gcc 5 supports generating the mcount tables directly
> >    ifeq ($(call cc-option-yn,-mrecord-mcount),y)
> > -    CC_FLAGS_FTRACE	+= -mrecord-mcount
> > +    cc_flags_ftrace_	+= -mrecord-mcount
> >      export CC_USING_RECORD_MCOUNT := 1
> >    endif
> >    ifdef CONFIG_HAVE_NOP_MCOUNT
> >      ifeq ($(call cc-option-yn, -mnop-mcount),y)
> > -      CC_FLAGS_FTRACE	+= -mnop-mcount
> > +      cc_flags_ftrace_	+= -mnop-mcount
> >        CC_FLAGS_USING	+= -DCC_USING_NOP_MCOUNT
> >      endif
> >    endif
> >  endif
> >  ifdef CONFIG_HAVE_FENTRY
> >    ifeq ($(call cc-option-yn, -mfentry),y)
> > -    CC_FLAGS_FTRACE	+= -mfentry
> > +    cc_flags_ftrace_	+= -mfentry
> >      CC_FLAGS_USING	+= -DCC_USING_FENTRY
> >    endif
> >  endif
> > +
> > +ifndef CC_FLAGS_FTRACE
> > +  CC_FLAGS_FTRACE := $(cc_flags_ftrace_)
> > +endif

Unfortunately this changes logic and inconsistent. With that change
platforms which define CC_FLAGS_FTRACE in arch/$(SRCARCH)/Makefile
would end up without extra -mrecord-mcount, -mnop-mcount and -mfentry
build flags, while CC_FLAGS_USING would be populated and
CC_USING_RECORD_MCOUNT set.

> >  export CC_FLAGS_FTRACE
> >  KBUILD_CFLAGS	+= $(CC_FLAGS_FTRACE) $(CC_FLAGS_USING)
> >  KBUILD_AFLAGS	+= $(CC_FLAGS_USING)
>