All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>,
	David Ahern <dsahern@gmail.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Thomas Gleixner <tglx@linutronix.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Jiri Olsa <jolsa@redhat.com>
Subject: Re: [PATCH] perf auto-dep: Speed up feature tests by building them in parallel
Date: Wed, 2 Oct 2013 08:28:35 +0200	[thread overview]
Message-ID: <20131002062835.GE31122@gmail.com> (raw)
In-Reply-To: <87zjqsmcma.fsf@sejong.aot.lge.com>


* 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

  reply	other threads:[~2013-10-02  6:28 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-12 13:38 [GIT PULL] perf fixes Ingo Molnar
2013-09-12 18:03 ` Linus Torvalds
2013-09-12 18:10   ` Linus Torvalds
2013-09-12 18:43     ` Arnaldo Carvalho de Melo
2013-09-12 19:12       ` Arnaldo Carvalho de Melo
2013-09-12 19:13         ` Linus Torvalds
2013-09-12 19:55       ` Ingo Molnar
2013-09-12 19:58       ` David Ahern
2013-09-12 20:02         ` Arnaldo Carvalho de Melo
2013-09-12 20:31           ` Ingo Molnar
2013-09-12 20:43             ` Ingo Molnar
2013-09-15  9:10               ` [PATCH] perf test-hack: Split out feature tests to cache them and to build them in parallel Ingo Molnar
2013-09-30 16:42                 ` [PATCH] perf auto-dep: Speed up feature tests by building " Ingo Molnar
2013-09-30 17:12                   ` Arnaldo Carvalho de Melo
2013-09-30 17:27                     ` Arnaldo Carvalho de Melo
2013-09-30 17:30                       ` Arnaldo Carvalho de Melo
2013-09-30 17:36                         ` Arnaldo Carvalho de Melo
2013-09-30 17:39                           ` Arnaldo Carvalho de Melo
2013-09-30 17:46                             ` Arnaldo Carvalho de Melo
2013-09-30 18:02                               ` Arnaldo Carvalho de Melo
2013-09-30 19:15                                 ` Ingo Molnar
2013-09-30 19:09                         ` Ingo Molnar
2013-09-30 17:34                     ` Linus Torvalds
2013-09-30 17:53                       ` Arnaldo Carvalho de Melo
2013-09-30 19:04                         ` Ingo Molnar
2013-10-01 11:34                           ` [PATCH] perf autodep: Remove strlcpy feature check, add __weak strlcpy implementation Ingo Molnar
2013-10-01 12:04                             ` Ingo Molnar
2013-10-01 12:48                               ` Arnaldo Carvalho de Melo
2013-10-01 12:51                               ` [PATCH] perf autodep: Speed up the 'all features are present' case Ingo Molnar
2013-10-01 14:46                             ` [PATCH] perf tools: Speed up git-version test on re-make Ingo Molnar
2013-10-02  6:47                               ` Namhyung Kim
2013-10-02  6:50                                 ` Ingo Molnar
2013-10-02  8:04                                   ` Namhyung Kim
2013-10-01 15:27                             ` [PATCH] perf autodep: Remove strlcpy feature check, add __weak strlcpy implementation Ingo Molnar
2013-10-01 15:29                               ` [PATCH] perf tools: Speed up the final link Ingo Molnar
2013-10-01  7:04                       ` [PATCH] perf auto-dep: Speed up feature tests by building them in parallel Geert Uytterhoeven
2013-10-01  8:38                         ` Ingo Molnar
2013-10-02  6:05                   ` Namhyung Kim
2013-10-02  6:28                     ` Ingo Molnar [this message]
2013-10-02  9:26                   ` Jiri Olsa
2013-10-02 10:11                     ` Ingo Molnar
2013-09-12 20:18         ` [GIT PULL] perf fixes Ingo Molnar
2013-09-12 20:38           ` Arnaldo Carvalho de Melo
2013-09-12 20:46             ` Ingo Molnar
2013-09-12 21:09               ` David Ahern
2013-09-12 21:18                 ` Ingo Molnar
2013-09-12 22:10                   ` David Ahern
2013-09-13  5:09                     ` Ingo Molnar
2013-09-13  9:32                       ` Jean Pihet
2013-09-13  9:45                         ` Ingo Molnar
2013-09-13 17:15                           ` Jean Pihet
2013-09-12 18:51     ` Linus Torvalds
2013-09-12 20:33       ` Ingo Molnar
2013-09-12 20:38         ` Linus Torvalds
2013-09-12 20:49           ` Ingo Molnar
2013-09-12 20:52             ` Linus Torvalds
2013-09-12 21:01               ` Ingo Molnar
2013-09-12 20:10     ` Ingo Molnar
2013-10-02  7:31   ` [PATCH] tools/perf: Fix double/triple-build of the feature detection logic during 'make install' et al Ingo Molnar
2013-10-02  9:28     ` [PATCH] tools/perf/build: Automatically build in parallel, based on number of CPUs in the system Ingo Molnar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20131002062835.GE31122@gmail.com \
    --to=mingo@kernel.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=acme@ghostprotocols.net \
    --cc=akpm@linux-foundation.org \
    --cc=dsahern@gmail.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=namhyung@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.