Re: [PATCH] perf auto-dep: Speed up feature tests by building them in parallel

[Ingo Molnar <mingo@kernel.org>]

* Namhyung Kim <namhyung@kernel.org> wrote:

> Hi Ingo,
> 
> On Mon, 30 Sep 2013 18:42:10 +0200, Ingo Molnar wrote:
> > This series (with combo patch attached) implements (much) faster 
> > perf-tools feature-auto-detection.
> >
> > I used 3 tricks to implement feature auto-dependencies and to speed up 
> > feature detection:
> >
> >   - standalone Makefile in config/feature-checks/ built in parallel
> >
> >   - split-out standalone .c files in config/feature-checks/*.c
> >
> >   - used GCC's auto-dependency generation feature (-MD) to track the
> >     effects of system library addition/removal.
> 
> I have a memory that this could lead to a nasty build failure.  Please 
> see the commit b6f4f804108b ("tools lib traceevent: Do not generate 
> dependency for system header files").

I think that at least the 'make clean' failure was just a buggy Makefile. 
To quote the build error from the commit:

   comet:~/tip/tools/lib/traceevent> make clean
   make: *** No rule to make target `/usr/lib/gcc/x86_64-redhat-linux/4.7.0/include/stddef.h', needed by `.trace-seq

It suggests that the 'clean' target depended on .d dependency files - 
that's a fundamentally incorrect use of -M/-MD auto-dependencies.

> The problem is that it turned out to depend on some compiler headers 
> which are located under some directory with a version number.  If so, 
> when compiler upgraded to a new version, it cannot find the original 
> dependencies so fail to build.
> 
>  $ cat config/feature-checks/test-libelf.d
>   test-libelf: test-libelf.c /usr/include/libelf.h /usr/include/sys/types.h \
>    /usr/include/features.h /usr/include/stdc-predef.h \
>    /usr/include/sys/cdefs.h /usr/include/bits/wordsize.h \
>    /usr/include/gnu/stubs.h /usr/include/gnu/stubs-64.h \
>    /usr/include/bits/types.h /usr/include/bits/typesizes.h \
>    /usr/include/time.h \
>    /usr/lib/gcc/x86_64-redhat-linux/4.7.2/include/stddef.h \
>    /usr/include/endian.h /usr/include/bits/endian.h \
>    /usr/include/bits/byteswap.h /usr/include/bits/byteswap-16.h \
>    /usr/include/sys/select.h /usr/include/bits/select.h \
>    /usr/include/bits/sigset.h /usr/include/bits/time.h \
>    /usr/include/sys/sysmacros.h /usr/include/bits/pthreadtypes.h \
>    /usr/include/elf.h \
>    /usr/lib/gcc/x86_64-redhat-linux/4.7.2/include/stdint.h \
>    /usr/include/stdint.h /usr/include/bits/wchar.h
> 
> In this case we are using this for feature-checking, so I guess it'd 
> fail to check the feature after upgrade.

The dependencies are re-made by GCC if a target fails and is rebuilt - and 
that should include the new header locations.

I checked out the parent commit (8f7c1d07ade5) which still had full -M, 
and this is how it utilized dependencies:

# let .d file also depends on the source and header files
define check_deps
                @set -e; $(RM) $@; \
                $(CC) -M $(CFLAGS) $< > $@.$$$$; \
                sed 's,\($*\)\.o[ :]*,\1.o $@ : ,g' < $@.$$$$ > $@; \
                $(RM) $@.$$$$
endef

that's not a very robust method either: .d files should be generated via 
-MD not via -M and should be included directly into the Makefile, like I 
did it in my patch:

-include *.d */*.d

and the .d files themselves are never added as dependencies - they are 
re-made by compilation automatically, not by any explicit Makefile rule.  
Adding them as dependencies risks circular dependencies, because the only 
method to rebuild a .d file is to actually meet the dependencies of a .c 
target.

So if done properly I don't think the build failure cited in that 
changelog can trigger.

Now, I cannot vouch for -MD blindly, without having seen a lot more 
testing, so we might still be forced to disable or limit that auto-dep 
trick, but the reasons cited in b6f4f804108b don't seem to be a GCC bug 
but a Makefile bug - they just weren't fully understood back then.

Thanks,

	Ingo