{"thread":{"id":"27652","subject":"[PATCH 1/3] Add option to disable NORETURN","startedAt":"2011-06-19T01:07:03Z","lastAt":"2011-06-21T05:00:03Z","messageCount":20,"participants":["Andi Kleen","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"170225","messageId":"1308445625-30667-1-git-send-email-andi@firstfloor.org","threadId":"27652","inReplyTo":null,"subject":"[PATCH 1/3] Add option to disable NORETURN","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-19T01:07:03Z","receivedAt":"2011-06-19T01:07:03Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Junio C Hamano <gitster@pobox.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\nThis adds a Makefile variable to disable noreturns.\n\n[Patch by Junio, description by Andi Kleen]\n\nSigned-off-by: Andi Kleen <ak@linux.intel.com>\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 e40ac0c..03b4499 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -153,6 +153,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@@ -1374,6 +1377,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 e0bb81e..9925cf0 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-- \n1.7.4.4\n"},{"id":"170227","messageId":"1308445625-30667-2-git-send-email-andi@firstfloor.org","threadId":"27652","inReplyTo":"1308445625-30667-1-git-send-email-andi@firstfloor.org","subject":"[PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-19T01:07:04Z","receivedAt":"2011-06-19T01:07:04Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"From: Andi Kleen <ak@linux.intel.com>\n\nAdd a lot of dummy returns to silence \"control flow reaches\nend of non void function\" warnings with disabled noreturn.\n\nIf NO_NORETURN is not disabled they will be all optimized away.\n\nSigned-off-by: Andi Kleen <ak@linux.intel.com>\n---\n builtin/blame.c          |    1 +\n builtin/bundle.c         |    1 +\n builtin/commit.c         |    1 +\n builtin/fetch-pack.c     |    1 +\n builtin/grep.c           |    1 +\n builtin/help.c           |    1 +\n builtin/pack-redundant.c |    1 +\n builtin/push.c           |    3 +--\n builtin/reflog.c         |    1 +\n config.c                 |    1 +\n connect.c                |    1 +\n daemon.c                 |    1 +\n date.c                   |    1 +\n diff.c                   |    1 +\n notes-merge.c            |    1 +\n object.c                 |    1 +\n parse-options.c          |    1 +\n read-cache.c             |    1 +\n remote.c                 |    1 +\n revision.c               |    1 +\n setup.c                  |    1 +\n shell.c                  |    1 +\n submodule.c              |    1 +\n transport.c              |    1 +\n 24 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 26a5d42..aff7781 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2011,6 +2011,7 @@ static const char *parse_loc(const char *spec,\n \t\tregerror(reg_error, &regexp, errbuf, 1024);\n \t\tdie(\"-L parameter '%s': %s\", spec + 1, errbuf);\n \t}\n+\treturn NULL;\n }\n \n /*\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 81046a9..14da7c3 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -62,4 +62,5 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)\n \t\t\tlist_bundle_refs(&header, argc, argv);\n \t} else\n \t\tusage(builtin_bundle_usage);\n+\treturn 0;\n }\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5286432..51ee2e5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -962,6 +962,7 @@ static const char *find_author_by_nickname(const char *name)\n \t\treturn strbuf_detach(&buf, NULL);\n \t}\n \tdie(_(\"No existing author found with '%s'\"), name);\n+\treturn NULL;\n }\n \n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 4367984..c81855c 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -208,6 +208,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n \t\t}\n \t}\n \tdie(\"git fetch_pack: expected ACK/NAK, got '%s'\", line);\n+\treturn ACK;\n }\n \n static void send_request(int fd, struct strbuf *buf)\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 871afaa..3637a72 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -607,6 +607,7 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\treturn hit;\n \t}\n \tdie(_(\"unable to grep from object of type %s\"), typename(obj->type));\n+\treturn 0;\n }\n \n static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 61ff798..5132f58 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -55,6 +55,7 @@ static enum help_format parse_help_format(const char *format)\n \tif (!strcmp(format, \"web\") || !strcmp(format, \"html\"))\n \t\treturn HELP_FORMAT_WEB;\n \tdie(\"unrecognized help format '%s'\", format);\n+\treturn 0;\n }\n \n static const char *get_man_viewer_info(const char *name)\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex f5c6afc..bf70ecc 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -580,6 +580,7 @@ static struct pack_list * add_pack_file(const char *filename)\n \t\tp = p->next;\n \t}\n \tdie(\"Filename %s not found in packed_git\", filename);\n+\treturn NULL;\n }\n \n static void load_all(void)\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 9cebf9e..9ac4309 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -265,6 +265,5 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \trc = do_push(repo, flags);\n \tif (rc == -1)\n \t\tusage_with_options(push_usage, options);\n-\telse\n-\t\treturn rc;\n+\treturn rc;\n }\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex ebf610e..8e64b81 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -779,4 +779,5 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)\n \n \t/* Not a recognized reflog command..*/\n \tusage(reflog_usage);\n+\treturn 0;\n }\ndiff --git a/config.c b/config.c\nindex e0b3b80..aa9ef85 100644\n--- a/config.c\n+++ b/config.c\n@@ -324,6 +324,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n \t\t\tbreak;\n \t}\n \tdie(\"bad config file line %d in %s\", config_linenr, config_file_name);\n+\treturn 0;\n }\n \n static int parse_unit_factor(const char *end, unsigned long *val)\ndiff --git a/connect.c b/connect.c\nindex 2119c3f..ae64e8b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -148,6 +148,7 @@ static enum protocol get_protocol(const char *name)\n \tif (!strcmp(name, \"file\"))\n \t\treturn PROTO_LOCAL;\n \tdie(\"I don't handle protocol '%s'\", name);\n+\treturn PROTO_GIT;\n }\n \n #define STR_(s)\t# s\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..adc4cc1 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -930,6 +930,7 @@ static int service_loop(struct socketlist *socklist)\n \t\t\t}\n \t\t}\n \t}\n+\treturn 0;\n }\n \n /* if any standard file descriptor is missing open it to /dev/null */\ndiff --git a/date.c b/date.c\nindex 896fbb4..3db476f 100644\n--- a/date.c\n+++ b/date.c\n@@ -675,6 +675,7 @@ enum date_mode parse_date_format(const char *format)\n \t\treturn DATE_RAW;\n \telse\n \t\tdie(\"unknown date format %s\", format);\n+\treturn DATE_NORMAL;\n }\n \n void datestamp(char *buf, int bufsize)\ndiff --git a/diff.c b/diff.c\nindex 8f4815b..468b9de 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -506,6 +506,7 @@ static struct diff_tempfile *claim_diff_tempfile(void) {\n \t\tif (!diff_temp[i].name)\n \t\t\treturn diff_temp + i;\n \tdie(\"BUG: diff is failing to clean up its tempfiles\");\n+\treturn NULL;\n }\n \n static int remove_tempfile_installed;\ndiff --git a/notes-merge.c b/notes-merge.c\nindex e1aaf43..8eadb8a 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -462,6 +462,7 @@ static int merge_one_change(struct notes_merge_options *o,\n \t\treturn 0;\n \t}\n \tdie(\"Unknown strategy (%i).\", o->strategy);\n+\treturn 0;\n }\n \n static int merge_changes(struct notes_merge_options *o,\ndiff --git a/object.c b/object.c\nindex 31976b5..74f165d 100644\n--- a/object.c\n+++ b/object.c\n@@ -41,6 +41,7 @@ int type_from_string(const char *str)\n \t\tif (!strcmp(str, object_type_strings[i]))\n \t\t\treturn i;\n \tdie(\"invalid object type \\\"%s\\\"\", str);\n+\treturn 0;\n }\n \n static unsigned int hash_obj(struct object *obj, unsigned int n)\ndiff --git a/parse-options.c b/parse-options.c\nindex 73bd28a..533ea9e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -147,6 +147,7 @@ static int get_value(struct parse_opt_ctx_t *p,\n \tdefault:\n \t\tdie(\"should not happen, someone must be hit on the forehead\");\n \t}\n+\treturn 0;\n }\n \n static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options)\ndiff --git a/read-cache.c b/read-cache.c\nindex 4ac9a03..2058b7a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1360,6 +1360,7 @@ unmap:\n \tmunmap(mmap, mmap_size);\n \terrno = EINVAL;\n \tdie(\"index file corrupt\");\n+\treturn 0;\n }\n \n int is_index_unborn(struct index_state *istate)\ndiff --git a/remote.c b/remote.c\nindex ca42a12..e22b5d4 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -655,6 +655,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\treturn NULL;\n \t}\n \tdie(\"Invalid refspec '%s'\", refspec[i]);\n+\treturn NULL;\n }\n \n int valid_fetch_refspec(const char *fetch_refspec_str)\ndiff --git a/revision.c b/revision.c\nindex c46cfaa..e44083a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -255,6 +255,7 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object\n \t\treturn NULL;\n \t}\n \tdie(\"%s is unknown object\", name);\n+\treturn NULL;\n }\n \n static int everybody_uninteresting(struct commit_list *orig)\ndiff --git a/setup.c b/setup.c\nindex ce87900..873aa2c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -79,6 +79,7 @@ int check_filename(const char *prefix, const char *arg)\n \tif (errno == ENOENT || errno == ENOTDIR)\n \t\treturn 0; /* file does not exist */\n \tdie_errno(\"failed to stat '%s'\", arg);\n+\treturn 0;\n }\n \n static void NORETURN die_verify_filename(const char *prefix, const char *arg)\ndiff --git a/shell.c b/shell.c\nindex abb8622..23ed2ec 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -215,4 +215,5 @@ int main(int argc, char **argv)\n \t\tdie(\"invalid command format '%s': %s\", argv[2],\n \t\t    split_cmdline_strerror(count));\n \t}\n+\treturn 0;\n }\ndiff --git a/submodule.c b/submodule.c\nindex b6dec70..e177516 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -245,6 +245,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)\n \t\t\treturn RECURSE_SUBMODULES_ON_DEMAND;\n \t\tdie(\"bad %s argument: %s\", opt, arg);\n \t}\n+\treturn 0;\n }\n \n void show_submodule_summary(FILE *f, const char *path,\ndiff --git a/transport.c b/transport.c\nindex c9c8056..5113a64 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1131,6 +1131,7 @@ int transport_connect(struct transport *transport, const char *name,\n \t\treturn transport->connect(transport, name, exec, fd);\n \telse\n \t\tdie(\"Operation not supported by protocol\");\n+\treturn 0;\n }\n \n int transport_disconnect(struct transport *transport)\n-- \n1.7.4.4\n"},{"id":"170226","messageId":"1308445625-30667-3-git-send-email-andi@firstfloor.org","threadId":"27652","inReplyTo":"1308445625-30667-1-git-send-email-andi@firstfloor.org","subject":"[PATCH 3/3] Add profile feedback build to git v2","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-19T01:07:05Z","receivedAt":"2011-06-19T01:07:05Z","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\nv2: Set NO_NORETURN variable in build\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 03b4499..d00718f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2492,3 +2492,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 -DNO_NORETURN=1\n+PROFILE_USE_CFLAGS := $(CFLAGS) -fprofile-use -fprofile-correction -DNO_NORETURN=1\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":"170322","messageId":"7vsjr4b3tf.fsf@alter.siamese.dyndns.org","threadId":"27652","inReplyTo":"1308445625-30667-2-git-send-email-andi@firstfloor.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-20T21:17:32Z","receivedAt":"2011-06-20T21:17:32Z","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> Add a lot of dummy returns to silence \"control flow reaches\n> end of non void function\" warnings with disabled noreturn.\n>\n> If NO_NORETURN is not disabled they will be all optimized away.\n\nI think this is probably a bad move, given that the previous patch is a\ntemporary workaround until gcc 4.6 is fixed. With -Wunreachable-code on,\nthese will introduce noise for build without NO_NORETURN (either when\nprofile feedback is not used, or when profile feedback build is in use and\nit no longer requires the NO_NORETURN workaround).\n"},{"id":"170323","messageId":"20110620213001.GB32765@one.firstfloor.org","threadId":"27652","inReplyTo":"7vsjr4b3tf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-20T21:30:01Z","receivedAt":"2011-06-20T21:30:01Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"On Mon, Jun 20, 2011 at 02:17:32PM -0700, Junio C Hamano wrote:\n> Andi Kleen <andi@firstfloor.org> writes:\n> \n> > From: Andi Kleen <ak@linux.intel.com>\n> >\n> > Add a lot of dummy returns to silence \"control flow reaches\n> > end of non void function\" warnings with disabled noreturn.\n> >\n> > If NO_NORETURN is not disabled they will be all optimized away.\n> \n> I think this is probably a bad move, given that the previous patch is a\n\nThis is basically the patch you suggested. Do you have some other suggestion \nnow?\n\nFWIW I preferred my original minimal patch and I can go back to that one.\n\n> temporary workaround until gcc 4.6 is fixed. With -Wunreachable-code on,\n\ngcc mainline (and possibly 4.6.2) has it already fixed, but it's reasonable \nto assume 4.6.0 will be in use for a long time. There's nothing\n\"temporary\" about compiler workarounds, unless you wait 10 years\nor so.\n\n> these will introduce noise for build without NO_NORETURN (either when\n> profile feedback is not used, or when profile feedback build is in use and\n> it no longer requires the NO_NORETURN workaround).\n\nI fixed the noise in a followon patch. \n\n-Andi\n\n-- \nak@linux.intel.com -- Speaking for myself only.\n"},{"id":"170326","messageId":"7vk4cgb24p.fsf@alter.siamese.dyndns.org","threadId":"27652","inReplyTo":"7vsjr4b3tf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-20T21:53:58Z","receivedAt":"2011-06-20T21:53:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think this is probably a bad move, given that the previous patch is a\n> temporary workaround until gcc 4.6 is fixed. With -Wunreachable-code on,\n> these will introduce noise for build without NO_NORETURN (either when\n> profile feedback is not used, or when profile feedback build is in use and\n> it no longer requires the NO_NORETURN workaround).\n\nI would need to clarify with s/introduce noise/introduce more noise/; the\nexisting codebase is not noise-free.\n\nBut I do not see much point in making things worse, only to squelch\n\"reaches end of non void function\" warnings that will be given under the\nNO_NORETURN workaround configuration.\n"},{"id":"170327","messageId":"7vfwn4b1vb.fsf@alter.siamese.dyndns.org","threadId":"27652","inReplyTo":"20110620213001.GB32765@one.firstfloor.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-20T21:59:36Z","receivedAt":"2011-06-20T21:59:36Z","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> On Mon, Jun 20, 2011 at 02:17:32PM -0700, Junio C Hamano wrote:\n>> Andi Kleen <andi@firstfloor.org> writes:\n>> \n>> > From: Andi Kleen <ak@linux.intel.com>\n>> >\n>> > Add a lot of dummy returns to silence \"control flow reaches\n>> > end of non void function\" warnings with disabled noreturn.\n>> >\n>> > If NO_NORETURN is not disabled they will be all optimized away.\n>> \n>> I think this is probably a bad move, given that the previous patch is a\n>\n> This is basically the patch you suggested. Do you have some other suggestion \n> now?\n\nSorry, I do not recall suggesting to add these dummy returns. The NO_NORETURN\nworkaround (your [1/3]) is what I remember.\n\n>> these will introduce noise for build without NO_NORETURN (either when\n>> profile feedback is not used, or when profile feedback build is in use and\n>> it no longer requires the NO_NORETURN workaround).\n>\n> I fixed the noise in a followon patch. \n\nI suspect that we are talking about different warnings.\n\nThe extra unreachable returns this patch adds will introduce more\n\"unreachable code\" warnings, which was what my message you are responding\nto is about.\n\nWhile I agree with you that this patch will now squelch \"control flow\nreaches end of non void function\" warnings (which is your justification\nfor this patch), I am just pointing out that it is like robbing Peter to\npay Paul.\n"},{"id":"170328","messageId":"20110620220027.GD32765@one.firstfloor.org","threadId":"27652","inReplyTo":"7vk4cgb24p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-20T22:00:27Z","receivedAt":"2011-06-20T22:00:27Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"> I would need to clarify with s/introduce noise/introduce more noise/; the\n> existing codebase is not noise-free.\n> \n> But I do not see much point in making things worse, only to squelch\n> \"reaches end of non void function\" warnings that will be given under the\n> NO_NORETURN workaround configuration.\n\nCan you please give specific guidance what I should do to make\nthe patchkit acceptable?\n\nCurrent options are:\n\n1) use original minimal patchkit (which had two warnings or so)\n1b) use original minimal patchkit with warnings fixed\n2) use global patch proposal for NO_NORETURN (= lots of warnings)\n2b) use patch proposal + additional patch to fix warnings (posted here)\n3) something I missed.\n\nWhich one do you prefer? If 3 I would prefer specific guidance.\n\nThanks,\n\n-Andi\n\n-- \nak@linux.intel.com -- Speaking for myself only.\n"},{"id":"170329","messageId":"20110620220347.GE32765@one.firstfloor.org","threadId":"27652","inReplyTo":"7vfwn4b1vb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-20T22:03:47Z","receivedAt":"2011-06-20T22:03:47Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"> Sorry, I do not recall suggesting to add these dummy returns. The NO_NORETURN\n> workaround (your [1/3]) is what I remember.\n\nIt was just a logical followup to quelch the warning storm NO_NORETURN \ncaused.\n\n> \n> >> these will introduce noise for build without NO_NORETURN (either when\n> >> profile feedback is not used, or when profile feedback build is in use and\n> >> it no longer requires the NO_NORETURN workaround).\n> >\n> > I fixed the noise in a followon patch. \n> \n> I suspect that we are talking about different warnings.\n> \n> The extra unreachable returns this patch adds will introduce more\n> \"unreachable code\" warnings, which was what my message you are responding\n> to is about.\n\nHmm I didn't see any additional warnings from the patch, neither\nin profile feedback nor in a normal build with gcc 4.5. Did I miss\nsomething?\n\n-Andi\n"},{"id":"170330","messageId":"7vboxsb0f3.fsf@alter.siamese.dyndns.org","threadId":"27652","inReplyTo":"20110620220027.GD32765@one.firstfloor.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-20T22:30:56Z","receivedAt":"2011-06-20T22:30:56Z","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>> I would need to clarify with s/introduce noise/introduce more noise/; the\n>> existing codebase is not noise-free.\n>> \n>> But I do not see much point in making things worse, only to squelch\n>> \"reaches end of non void function\" warnings that will be given under the\n>> NO_NORETURN workaround configuration.\n>\n> Can you please give specific guidance what I should do to make\n> the patchkit acceptable?\n\nI am inclined to apply only 1 and 3, which is what I already have on 'pu'.\n"},{"id":"170331","messageId":"20110620223156.GA695@elie","threadId":"27652","inReplyTo":"20110620220347.GE32765@one.firstfloor.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-20T22:31:56Z","receivedAt":"2011-06-20T22:31:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Andi,\n\nAndi Kleen wrote:\n\n>> Sorry, I do not recall suggesting to add these dummy returns. The NO_NORETURN\n>> workaround (your [1/3]) is what I remember.\n>\n> It was just a logical followup to quelch the warning storm NO_NORETURN \n> caused.\n\nPlease remember to think for yourself. ;-)  Junio generally gives good\nadvice, but if you don't see the wisdom in it, that's the time to ask\nquestions, not blindly do a wrong thing.\n\nIn this case, since the NO_NORETURN knob is to work around a gcc bug,\nwouldn't it make sense to add a -Wno-something-or-other option to\nBASIC_CFLAGS or COMPAT_CFLAGS when it is set?\n\nHope that helps,\nJonathan\n"},{"id":"170332","messageId":"20110620223322.GF32765@one.firstfloor.org","threadId":"27652","inReplyTo":"7vboxsb0f3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-20T22:33:22Z","receivedAt":"2011-06-20T22:33:22Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"> I am inclined to apply only 1 and 3, which is what I already have on 'pu'.\n\nThanks. \n\nI guess should add some documentation that explains that there\nare additional warnings (and some general pointers). I'll send that\nin another patch.\n\n-Andi\n> \n\n-- \nak@linux.intel.com -- Speaking for myself only.\n"},{"id":"170333","messageId":"20110620223705.GG32765@one.firstfloor.org","threadId":"27652","inReplyTo":"20110620223156.GA695@elie","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Andi Kleen","fromEmail":"andi@firstfloor.org","sentAt":"2011-06-20T22:37:05Z","receivedAt":"2011-06-20T22:37:05Z","isPatch":true,"sender":{"key":"andi@firstfloor.org","avatar":null},"body":"> Please remember to think for yourself. ;-)  Junio generally gives good\n> advice, but if you don't see the wisdom in it, that's the time to ask\n> questions, not blindly do a wrong thing.\n\nTo be honest it's still not clear to me what was wrong with patch (2).\n\n> \n> In this case, since the NO_NORETURN knob is to work around a gcc bug,\n> wouldn't it make sense to add a -Wno-something-or-other option to\n> BASIC_CFLAGS or COMPAT_CFLAGS when it is set?\n\nThe problem is that only relatively new gccs have options to do\nfine grained control on all warnings. So this would add more complications\nin the Makefile to check the compiler version\n\n(I haven't checked if that's true for this particular warning,\nbut I have run into this several times in the past for others)\n\n-Andi\n\n-- \nak@linux.intel.com -- Speaking for myself only.\n"},{"id":"170335","messageId":"20110620224619.GB695@elie","threadId":"27652","inReplyTo":"20110620223705.GG32765@one.firstfloor.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-20T22:46:19Z","receivedAt":"2011-06-20T22:46:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Andi Kleen wrote:\n\n> To be honest it's still not clear to me what was wrong with patch (2).\n\nAh, ok.  From my point of view the patch was problematic since it adds\ncode that would be distracting to both humans and static analyzers.\nFor example, a person might wonder \"why this return value and not\nanother?\".  Even worse, if someone removes a die() call and introduces\na bug, we lose the benefit of the warnings you are suppressing.\n\n>> In this case, since the NO_NORETURN knob is to work around a gcc bug,\n>> wouldn't it make sense to add a -Wno-something-or-other option to\n>> BASIC_CFLAGS or COMPAT_CFLAGS when it is set?\n>\n> The problem is that only relatively new gccs have options to do\n> fine grained control on all warnings. So this would add more complications\n> in the Makefile to check the compiler version\n\nThe default CFLAGS is very simple because git is supposed to be\npossible to build with other compilers, too, and I wasn't suggesting\nchanging that.  But I suspect making NO_NORETURN assume a modern\nenough gcc to trigger the bug might be ok.\n\nAnyway, thanks for writing these patches.  I'm happy to see git get\nfaster.  As a side question, do you know if gcc provides a way to\nprint output about what profile-driven optimizations were especially\ncompelling, so they could help people think about how to reorganize\ncode to improve the non profile-driven builds, too?\n\nRegards,\nJonathan\n"},{"id":"170336","messageId":"20110620224827.GC695@elie","threadId":"27652","inReplyTo":"20110620224619.GB695@elie","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-20T22:48:28Z","receivedAt":"2011-06-20T22:48:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Even worse, if someone removes a die() call and introduces\n> a bug, we lose the benefit of the warnings you are suppressing.\n\n(Scratch that part --- I don't know what I was thinking.  Will\nthink more carefully before hitting \"send\" next time.)\n"},{"id":"170338","messageId":"7v1uyoaxu5.fsf@alter.siamese.dyndns.org","threadId":"27652","inReplyTo":"20110620223705.GG32765@one.firstfloor.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-20T23:26:42Z","receivedAt":"2011-06-20T23:26:42Z","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>> Please remember to think for yourself. ;-)  Junio generally gives good\n>> advice, but if you don't see the wisdom in it, that's the time to ask\n>> questions, not blindly do a wrong thing.\n>\n> To be honest it's still not clear to me what was wrong with patch (2).\n\nFor example.\n\n    diff --git a/builtin/commit.c b/builtin/commit.c\n    index 5286432..51ee2e5 100644\n    --- a/builtin/commit.c\n    +++ b/builtin/commit.c\n    @@ -962,6 +962,7 @@ static const char *find_author_by_nickname(const char *name)\n                    return strbuf_detach(&buf, NULL);\n            }\n            die(_(\"No existing author found with '%s'\"), name);\n    +\treturn NULL;\n     }\n\nWhen the above is applied and compiled without NO_NORETURN, the extra\nreturn may be optimized out by the compiler as your commit log messages\nsaid, but wouldn't it introduce a new warning:\n\n  builtin/commit.c: In function 'find_author_by_nickname':\n  builtin/commit.c:965: error: will never be executed\n\nunder -Wunreachable-code?\n"},{"id":"170341","messageId":"20110621001703.GA700@alboin.amr.corp.intel.com","threadId":"27652","inReplyTo":"7v1uyoaxu5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Andi Kleen","fromEmail":"ak@linux.intel.com","sentAt":"2011-06-21T00:17:03Z","receivedAt":"2011-06-21T00:17:03Z","isPatch":true,"sender":{"key":"ak@linux.intel.com","avatar":null},"body":"> When the above is applied and compiled without NO_NORETURN, the extra\n> return may be optimized out by the compiler as your commit log messages\n> said, but wouldn't it introduce a new warning:\n> \n>   builtin/commit.c: In function 'find_author_by_nickname':\n>   builtin/commit.c:965: error: will never be executed\n> \n> under -Wunreachable-code?\n\nIt may, but that option isn't set for git?\n\n-Andi\n"},{"id":"170343","messageId":"20110621002448.GB700@alboin.amr.corp.intel.com","threadId":"27652","inReplyTo":"20110620224619.GB695@elie","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Andi Kleen","fromEmail":"ak@linux.intel.com","sentAt":"2011-06-21T00:24:48Z","receivedAt":"2011-06-21T00:24:48Z","isPatch":true,"sender":{"key":"ak@linux.intel.com","avatar":null},"body":"> Anyway, thanks for writing these patches.  I'm happy to see git get\n> faster.  As a side question, do you know if gcc provides a way to\n> print output about what profile-driven optimizations were especially\n> compelling, so they could help people think about how to reorganize\n> code to improve the non profile-driven builds, too?\n\nGenerally gcc has no idea how much difference an optimization makes.\nIt would need to run the code for that, but it doesn't.\n\nThat's generally only possible for JITs.\n\nFor some optimizations (basic block reordering) you could get\nthe same benefit with __builtin_expect.\n\nBut based on my own experience with __builtin_expect in other projects\nI strongly recommend to not use it manually: people tend\nto use it everywhere eventually and they often get it wrong.\nHumans are quite bad at deciding such things. Also code behaviour\nchanges over time and then the annotations often become outdated.\n\n[e.g. the kernel has a special profiler for builtin_expects --\naka unlikely -- which checks the manual annotation against\nthe true runtime behaviour and the failure rate of manual annotation \nis quite spectacular]\n\nIn addition there are various optimizations in gcc where I am not\naware of a manual annotation possibility (like register allocation). \nThe data from profile feedback is used in quite a lot of places all\nover the compiler.\n\n-Andi\n"},{"id":"170348","messageId":"7vsjr3akmq.fsf@alter.siamese.dyndns.org","threadId":"27652","inReplyTo":"20110620223322.GF32765@one.firstfloor.org","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-21T04:11:57Z","receivedAt":"2011-06-21T04:11:57Z","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>> I am inclined to apply only 1 and 3, which is what I already have on 'pu'.\n>\n> Thanks. \n\nThank _you_ for working on this.\n\n> I guess should add some documentation that explains that there\n> are additional warnings (and some general pointers). I'll send that\n> in another patch.\n\nYup, applied. Thanks.\n"},{"id":"170349","messageId":"20110621050003.GB16763@elie","threadId":"27652","inReplyTo":"20110621002448.GB700@alboin.amr.corp.intel.com","subject":"Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-06-21T05:00:03Z","receivedAt":"2011-06-21T05:00:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Andi Kleen wrote:\n\n> For some optimizations (basic block reordering) you could get\n> the same benefit with __builtin_expect.\n>\n> But based on my own experience with __builtin_expect in other projects\n> I strongly recommend to not use it manually:\n[...]\n> In addition there are various optimizations in gcc where I am not\n> aware of a manual annotation possibility (like register allocation). \n> The data from profile feedback is used in quite a lot of places all\n> over the compiler.\n\nThanks.  Alas.\n"}]}