{"thread":{"id":"27586","subject":"[PATCH 2/2] Add profile feedback build to git","startedAt":"2011-06-08T21:43:36Z","lastAt":"2011-06-09T21:13:56Z","messageCount":7,"participants":["Andi Kleen","Junio C Hamano","Jeff King","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"169700","messageId":"1307569417-8924-1-git-send-email-andi@firstfloor.org","threadId":"27586","inReplyTo":null,"subject":"[PATCH 1/2] Remove noreturn function pointers in usage.c","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-08T21:43:36Z","receivedAt":"2011-06-08T21:43:36Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Andi Kleen <ak@linux.intel.com>\n\nDue to a bug in gcc 4.6+ it can crash when doing profile feedback\nwith a noreturn function pointer\n\n(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)\n\nRemove the NORETURNs from the die functions for now to work\naround this. Doesn't seem to make any difference.\n\nSigned-off-by: Andi Kleen <ak@linux.intel.com>\n---\n usage.c |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/usage.c b/usage.c\nindex b5e67e3..4045574 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -12,13 +12,13 @@ void vreportf(const char *prefix, const char *err, va_list params)\n \tfprintf(stderr, \"%s%s\\n\", prefix, msg);\n }\n \n-static NORETURN void usage_builtin(const char *err, va_list params)\n+static  void usage_builtin(const char *err, va_list params)\n {\n \tvreportf(\"usage: \", err, params);\n \texit(129);\n }\n \n-static NORETURN void die_builtin(const char *err, va_list params)\n+static  void die_builtin(const char *err, va_list params)\n {\n \tvreportf(\"fatal: \", err, params);\n \texit(128);\n@@ -36,8 +36,8 @@ static void warn_builtin(const char *warn, va_list params)\n \n /* If we are in a dlopen()ed .so write to a global variable would segfault\n  * (ugh), so keep things static. */\n-static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;\n-static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;\n+static void (*usage_routine)(const char *err, va_list params) = usage_builtin;\n+static void (*die_routine)(const char *err, va_list params) = die_builtin;\n static void (*error_routine)(const char *err, va_list params) = error_builtin;\n static void (*warn_routine)(const char *err, va_list params) = warn_builtin;\n \n-- \n1.7.4.4\n"},{"id":"169699","messageId":"1307569417-8924-2-git-send-email-andi@firstfloor.org","threadId":"27586","inReplyTo":"1307569417-8924-1-git-send-email-andi@firstfloor.org","subject":"[PATCH 2/2] Add profile feedback build to git","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-08T21:43:37Z","receivedAt":"2011-06-08T21:43:37Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Andi Kleen <ak@linux.intel.com>\n\nAdd a gcc profile feedback build option \"profile-all\" to the\nmain Makefile. It simply runs the test suite to generate feedback\ndata and the recompiles the main executables with that. The basic\nstructure is similar to the existing gcov code.\n\ngcc is often able to generate better code with profile feedback\ndata. The training load also doesn't need to be too similar\nto the actual load, it still gives benefits.\n\nThe test suite run is unfortunately quite long. It would\nbe good to find a suitable subset that runs faster and still\ngives reasonable feedback.\n\nFor now the test suite runs single threaded (I had some\ntrouble running the test suite with -jX)\n\nI tested it with git gc and git blame kernel/sched.c on a Linux\nkernel tree. For gc I get about 2.7% improvement in wall clock\ntime by using the feedback build, for blame about 2.4%.\nThat's not gigantic, but not shabby either for a very small patch.\n\nIf anyone has any favourite CPU intensive git benchmarks feel\nfree to try them too.\n\nI hope distributors will switch to use a feedback build in their\npackages.\n\nSigned-off-by: Andi Kleen <ak@linux.intel.com>\n---\n Makefile |   17 +++++++++++++++++\n 1 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex e40ac0c..85e2679 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2486,3 +2486,20 @@ cover_db: coverage-report\n \n cover_db_html: cover_db\n \tcover -report html -outputdir cover_db_html cover_db\n+\n+### profile feedback build\n+#\n+.PHONY: profile-all profile-clean\n+\n+PROFILE_GEN_CFLAGS := $(CFLAGS) -fprofile-generate\n+PROFILE_USE_CFLAGS := $(CFLAGS) -fprofile-use -fprofile-correction\n+\n+profile-clean:\n+\t$(RM) $(addsuffix *.gcda,$(object_dirs))\n+\t$(RM) $(addsuffix *.gcno,$(object_dirs))\n+\n+profile-all: profile-clean\n+\t$(MAKE) CFLAGS=\"$(PROFILE_GEN_CFLAGS)\" all\n+\t$(MAKE) CFLAGS=\"$(PROFILE_GEN_CFLAGS)\" -j1 test\n+\t$(MAKE) CFLAGS=\"$(PROFILE_USE_CFLAGS)\" all\n+\t\n-- \n1.7.4.4\n"},{"id":"169716","messageId":"7v4o3z264s.fsf@alter.siamese.dyndns.org","threadId":"27586","inReplyTo":"1307569417-8924-1-git-send-email-andi@firstfloor.org","subject":"Re: [PATCH 1/2] Remove noreturn function pointers in usage.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-09T00:36:03Z","receivedAt":"2011-06-09T00:36:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andi Kleen <andi@firstfloor.org> writes:\n\n> From: Andi Kleen <ak@linux.intel.com>\n>\n> Due to a bug in gcc 4.6+ it can crash when doing profile feedback\n> with a noreturn function pointer\n>\n> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)\n>\n> Remove the NORETURNs from the die functions for now to work\n> around this. Doesn't seem to make any difference.\n>\n> Signed-off-by: Andi Kleen <ak@linux.intel.com>\n\nI would expect a better patch from a well respected kernel person, though.\n\n - There are many more NORETURN and NORETURN_PTR in the code, and the\n   proposed commit log message does not explain why these two are the only\n   ones that are problematic and needs to be worked around. It does not\n   guide other people who might want to add NORETURN/NORETURN_PTR when\n   deciding if their change would break the \"fix\" this change brought in.\n\n - Potential impact to people who do not use Gcc 4.6 with profile feedback\n   is not explained away well, except for \"Doesn't seem to make any\n   difference.\"\n\n - If other NORETURN/NORETURN_PTR could/should also go (I don't know due\n   to the first bullet point above) when using the problematic compiler\n   with the profile feedback feature, wouldn't it be a better workaround\n   would be to introduce a Makefile variable to ask git-compat-util.h to\n   make these two a no-op, perhaps?\n\nA patch to do so may look like this.\n\nI did not like the triple negation \"make NO_NORETURN=NoThanks\" and wanted\nto name this AVOID_NORETURN instead, but decided to go with other existing\nMakefile variables.\n\n\n Makefile          |    6 ++++++\n git-compat-util.h |    2 +-\n 2 files changed, 7 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex d2e2ea1..70c814c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -144,6 +144,9 @@ all::\n # that tells runtime paths to dynamic libraries;\n # \"-Wl,-rpath=/path/lib\" is used instead.\n #\n+# Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,\n+# as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)\n+#\n # Define USE_NSEC below if you want git to care about sub-second file mtimes\n # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and\n # it will BREAK YOUR LOCAL DIFFS! show-diff and anything using it will likely\n@@ -1349,6 +1352,9 @@ endif\n ifdef USE_ST_TIMESPEC\n \tBASIC_CFLAGS += -DUSE_ST_TIMESPEC\n endif\n+ifdef NO_NORETURN\n+\tBASIC_CFLAGS += -DNO_NORETURN\n+endif\n ifdef NO_NSEC\n \tBASIC_CFLAGS += -DNO_NSEC\n endif\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 40498b3..13bc26f 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -218,7 +218,7 @@ extern char *gitbasename(char *);\n #if __HP_cc >= 61000\n #define NORETURN __attribute__((noreturn))\n #define NORETURN_PTR\n-#elif defined(__GNUC__)\n+#elif defined(__GNUC__) && !defined(NO_NORETURN)\n #define NORETURN __attribute__((__noreturn__))\n #define NORETURN_PTR __attribute__((__noreturn__))\n #elif defined(_MSC_VER)\n"},{"id":"169727","messageId":"20110609045915.GA15448@one.firstfloor.org","threadId":"27586","inReplyTo":"7v4o3z264s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Remove noreturn function pointers in usage.c","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-09T04:59:15Z","receivedAt":"2011-06-09T04:59:15Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":">  - There are many more NORETURN and NORETURN_PTR in the code, and the\n>    proposed commit log message does not explain why these two are the only\n>    ones that are problematic and needs to be worked around. It does not\n>    guide other people who might want to add NORETURN/NORETURN_PTR when\n>    deciding if their change would break the \"fix\" this change brought in.\n\nThis was the only place where it crashed the compiler.\n\nI don't have a good criterium to decide which cases do crash the compiler\nor not except for trying it.\n\nBut I believe the crash is relatively unlikely (needs quite a lot of conditions\nto line up), so it doesn't deserve extensive changes all over.\n\n> \n>  - Potential impact to people who do not use Gcc 4.6 with profile feedback\n>    is not explained away well, except for \"Doesn't seem to make any\n>    difference.\"\n\nI merely went by \"there are no new warnings\" (I assume that's the main\nmotivation)\n\n> \n>  - If other NORETURN/NORETURN_PTR could/should also go (I don't know due\n>    to the first bullet point above) when using the problematic compiler\n>    with the profile feedback feature, wouldn't it be a better workaround\n>    would be to introduce a Makefile variable to ask git-compat-util.h to\n>    make these two a no-op, perhaps?\n\nI don't think we need to remove the others for now.\n\n> \n> A patch to do so may look like this.\n> \n> I did not like the triple negation \"make NO_NORETURN=NoThanks\" and wanted\n> to name this AVOID_NORETURN instead, but decided to go with other existing\n> Makefile variables.\n\nGiven the explanation above (I can update the description with that)\ndo you still want the complete disabling?\n\n-Andi\n"},{"id":"169728","messageId":"20110609055224.GA27780@sigill.intra.peff.net","threadId":"27586","inReplyTo":"20110609045915.GA15448@one.firstfloor.org","subject":"Re: [PATCH 1/2] Remove noreturn function pointers in usage.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-09T05:52:24Z","receivedAt":"2011-06-09T05:52:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2011 at 06:59:15AM +0200, Andi Kleen wrote:\n\n> >  - Potential impact to people who do not use Gcc 4.6 with profile feedback\n> >    is not explained away well, except for \"Doesn't seem to make any\n> >    difference.\"\n> \n> I merely went by \"there are no new warnings\" (I assume that's the main\n> motivation)\n\nOn your compiler and settings, perhaps. With your patch I get:\n\n  usage.c: In function ‘die’:\n  usage.c:70:1: error: ‘noreturn’ function does return [-Werror]\n\nAnd rightfully so:\n\nvoid NORETURN die(const char *err, ...)\n{\n        va_list params;\n\n        va_start(params, err);\n        die_routine(err, params);\n        va_end(params);\n}\n\nYou've stripped the NORETURN from die_routine, so of course it looks like we\nend up returning.\n\nThis is with:\n\n  $ gcc --version | head -n 1\n  gcc (Debian 4.6.0-11) 4.6.1 20110604 (prerelease)\n\n-Peff\n"},{"id":"169731","messageId":"20110609063145.GC15448@one.firstfloor.org","threadId":"27586","inReplyTo":"20110609055224.GA27780@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] Remove noreturn function pointers in usage.c","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-09T06:31:45Z","receivedAt":"2011-06-09T06:31:45Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"On Thu, Jun 09, 2011 at 01:52:24AM -0400, Jeff King wrote:\n> On Thu, Jun 09, 2011 at 06:59:15AM +0200, Andi Kleen wrote:\n> \n> > >  - Potential impact to people who do not use Gcc 4.6 with profile feedback\n> > >    is not explained away well, except for \"Doesn't seem to make any\n> > >    difference.\"\n> > \n> > I merely went by \"there are no new warnings\" (I assume that's the main\n> > motivation)\n> \n> On your compiler and settings, perhaps. With your patch I get:\n> \n>   usage.c: In function ‘die’:\n>   usage.c:70:1: error: ‘noreturn’ function does return [-Werror]\n\nOk.  Hmm, all I can say it compiled here.\n\nOk then we have to remove it. I didn't really like Junio's approach\nto only do it for a single file because that would break with LTO / link\ntime optimization which requires declarations to match between \ntranslation units.\n\nMaybe it's better to stick an extra exit() at the end to shut up\nthe extra warning. I'll do that I guess.\n\nBTW 4.6.2 or so will have the problem fixed.\n\n-Andi\n"},{"id":"169814","messageId":"BANLkTimxA9yRHFt8jX2kEM8JvNoO5AOcYA@mail.gmail.com","threadId":"27586","inReplyTo":"20110609063145.GC15448@one.firstfloor.org","subject":"Re: [PATCH 1/2] Remove noreturn function pointers in usage.c","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-06-09T21:13:56Z","receivedAt":"2011-06-09T21:13:56Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Jun 9, 2011 at 8:31 AM, Andi Kleen <andi@firstfloor.org> wrote:\n> On Thu, Jun 09, 2011 at 01:52:24AM -0400, Jeff King wrote:\n>> On Thu, Jun 09, 2011 at 06:59:15AM +0200, Andi Kleen wrote:\n>>\n>> > >  - Potential impact to people who do not use Gcc 4.6 with profile feedback\n>> > >    is not explained away well, except for \"Doesn't seem to make any\n>> > >    difference.\"\n>> >\n>> > I merely went by \"there are no new warnings\" (I assume that's the main\n>> > motivation)\n>>\n>> On your compiler and settings, perhaps. With your patch I get:\n>>\n>>   usage.c: In function ‘die’:\n>>   usage.c:70:1: error: ‘noreturn’ function does return [-Werror]\n>\n> Ok.  Hmm, all I can say it compiled here.\n>\n> Ok then we have to remove it. I didn't really like Junio's approach\n> to only do it for a single file because that would break with LTO / link\n> time optimization which requires declarations to match between\n> translation units.\n\nJunio's approach didn't do it for a single file, it disabled the\nNORETURN mechanism all together, by having a Makefile-switch.\n\n> BTW 4.6.2 or so will have the problem fixed.\n\nIf it's only in 4.6 through 4.6.2, then we probably don't even need a\nMakefile-switch for junio's approach at all; just checking the GCC\nversion should be reliable enough, no?\n"}]}