[PATCH 1/3] Add option to disable NORETURN

[PATCH 1/3] Add option to disable NORETURN

[Andi Kleen]

From: Junio C Hamano <gitster@pobox.com>

Due to a bug in gcc 4.6+ it can crash when doing profile feedback
with a noreturn function pointer

(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)

This adds a Makefile variable to disable noreturns.

[Patch by Junio, description by Andi Kleen]

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 Makefile          |    6 ++++++
 git-compat-util.h |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index e40ac0c..03b4499 100644
--- a/Makefile
+++ b/Makefile
@@ -153,6 +153,9 @@ all::
 # that tells runtime paths to dynamic libraries;
 # "-Wl,-rpath=/path/lib" is used instead.
 #
+# Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
+# as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
+#
 # Define USE_NSEC below if you want git to care about sub-second file mtimes
 # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and
 # it will BREAK YOUR LOCAL DIFFS! show-diff and anything using it will likely
@@ -1374,6 +1377,9 @@ endif
 ifdef USE_ST_TIMESPEC
 	BASIC_CFLAGS += -DUSE_ST_TIMESPEC
 endif
+ifdef NO_NORETURN
+	BASIC_CFLAGS += -DNO_NORETURN
+endif
 ifdef NO_NSEC
 	BASIC_CFLAGS += -DNO_NSEC
 endif
diff --git a/git-compat-util.h b/git-compat-util.h
index e0bb81e..9925cf0 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -218,7 +218,7 @@ extern char *gitbasename(char *);
 #if __HP_cc >= 61000
 #define NORETURN __attribute__((noreturn))
 #define NORETURN_PTR
-#elif defined(__GNUC__)
+#elif defined(__GNUC__) && !defined(NO_NORETURN)
 #define NORETURN __attribute__((__noreturn__))
 #define NORETURN_PTR __attribute__((__noreturn__))
 #elif defined(_MSC_VER)
-- 
1.7.4.4

[PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Add a lot of dummy returns to silence "control flow reaches
end of non void function" warnings with disabled noreturn.

If NO_NORETURN is not disabled they will be all optimized away.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 builtin/blame.c          |    1 +
 builtin/bundle.c         |    1 +
 builtin/commit.c         |    1 +
 builtin/fetch-pack.c     |    1 +
 builtin/grep.c           |    1 +
 builtin/help.c           |    1 +
 builtin/pack-redundant.c |    1 +
 builtin/push.c           |    3 +--
 builtin/reflog.c         |    1 +
 config.c                 |    1 +
 connect.c                |    1 +
 daemon.c                 |    1 +
 date.c                   |    1 +
 diff.c                   |    1 +
 notes-merge.c            |    1 +
 object.c                 |    1 +
 parse-options.c          |    1 +
 read-cache.c             |    1 +
 remote.c                 |    1 +
 revision.c               |    1 +
 setup.c                  |    1 +
 shell.c                  |    1 +
 submodule.c              |    1 +
 transport.c              |    1 +
 24 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 26a5d42..aff7781 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2011,6 +2011,7 @@ static const char *parse_loc(const char *spec,
 		regerror(reg_error, &regexp, errbuf, 1024);
 		die("-L parameter '%s': %s", spec + 1, errbuf);
 	}
+	return NULL;
 }
 
 /*
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 81046a9..14da7c3 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -62,4 +62,5 @@ int cmd_bundle(int argc, const char **argv, const char *prefix)
 			list_bundle_refs(&header, argc, argv);
 	} else
 		usage(builtin_bundle_usage);
+	return 0;
 }
diff --git a/builtin/commit.c b/builtin/commit.c
index 5286432..51ee2e5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -962,6 +962,7 @@ static const char *find_author_by_nickname(const char *name)
 		return strbuf_detach(&buf, NULL);
 	}
 	die(_("No existing author found with '%s'"), name);
+	return NULL;
 }
 
 
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 4367984..c81855c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -208,6 +208,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 		}
 	}
 	die("git fetch_pack: expected ACK/NAK, got '%s'", line);
+	return ACK;
 }
 
 static void send_request(int fd, struct strbuf *buf)
diff --git a/builtin/grep.c b/builtin/grep.c
index 871afaa..3637a72 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -607,6 +607,7 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
 		return hit;
 	}
 	die(_("unable to grep from object of type %s"), typename(obj->type));
+	return 0;
 }
 
 static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
diff --git a/builtin/help.c b/builtin/help.c
index 61ff798..5132f58 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -55,6 +55,7 @@ static enum help_format parse_help_format(const char *format)
 	if (!strcmp(format, "web") || !strcmp(format, "html"))
 		return HELP_FORMAT_WEB;
 	die("unrecognized help format '%s'", format);
+	return 0;
 }
 
 static const char *get_man_viewer_info(const char *name)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index f5c6afc..bf70ecc 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -580,6 +580,7 @@ static struct pack_list * add_pack_file(const char *filename)
 		p = p->next;
 	}
 	die("Filename %s not found in packed_git", filename);
+	return NULL;
 }
 
 static void load_all(void)
diff --git a/builtin/push.c b/builtin/push.c
index 9cebf9e..9ac4309 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -265,6 +265,5 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	rc = do_push(repo, flags);
 	if (rc == -1)
 		usage_with_options(push_usage, options);
-	else
-		return rc;
+	return rc;
 }
diff --git a/builtin/reflog.c b/builtin/reflog.c
index ebf610e..8e64b81 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -779,4 +779,5 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
 
 	/* Not a recognized reflog command..*/
 	usage(reflog_usage);
+	return 0;
 }
diff --git a/config.c b/config.c
index e0b3b80..aa9ef85 100644
--- a/config.c
+++ b/config.c
@@ -324,6 +324,7 @@ static int git_parse_file(config_fn_t fn, void *data)
 			break;
 	}
 	die("bad config file line %d in %s", config_linenr, config_file_name);
+	return 0;
 }
 
 static int parse_unit_factor(const char *end, unsigned long *val)
diff --git a/connect.c b/connect.c
index 2119c3f..ae64e8b 100644
--- a/connect.c
+++ b/connect.c
@@ -148,6 +148,7 @@ static enum protocol get_protocol(const char *name)
 	if (!strcmp(name, "file"))
 		return PROTO_LOCAL;
 	die("I don't handle protocol '%s'", name);
+	return PROTO_GIT;
 }
 
 #define STR_(s)	# s
diff --git a/daemon.c b/daemon.c
index 4c8346d..adc4cc1 100644
--- a/daemon.c
+++ b/daemon.c
@@ -930,6 +930,7 @@ static int service_loop(struct socketlist *socklist)
 			}
 		}
 	}
+	return 0;
 }
 
 /* if any standard file descriptor is missing open it to /dev/null */
diff --git a/date.c b/date.c
index 896fbb4..3db476f 100644
--- a/date.c
+++ b/date.c
@@ -675,6 +675,7 @@ enum date_mode parse_date_format(const char *format)
 		return DATE_RAW;
 	else
 		die("unknown date format %s", format);
+	return DATE_NORMAL;
 }
 
 void datestamp(char *buf, int bufsize)
diff --git a/diff.c b/diff.c
index 8f4815b..468b9de 100644
--- a/diff.c
+++ b/diff.c
@@ -506,6 +506,7 @@ static struct diff_tempfile *claim_diff_tempfile(void) {
 		if (!diff_temp[i].name)
 			return diff_temp + i;
 	die("BUG: diff is failing to clean up its tempfiles");
+	return NULL;
 }
 
 static int remove_tempfile_installed;
diff --git a/notes-merge.c b/notes-merge.c
index e1aaf43..8eadb8a 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -462,6 +462,7 @@ static int merge_one_change(struct notes_merge_options *o,
 		return 0;
 	}
 	die("Unknown strategy (%i).", o->strategy);
+	return 0;
 }
 
 static int merge_changes(struct notes_merge_options *o,
diff --git a/object.c b/object.c
index 31976b5..74f165d 100644
--- a/object.c
+++ b/object.c
@@ -41,6 +41,7 @@ int type_from_string(const char *str)
 		if (!strcmp(str, object_type_strings[i]))
 			return i;
 	die("invalid object type \"%s\"", str);
+	return 0;
 }
 
 static unsigned int hash_obj(struct object *obj, unsigned int n)
diff --git a/parse-options.c b/parse-options.c
index 73bd28a..533ea9e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -147,6 +147,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 	default:
 		die("should not happen, someone must be hit on the forehead");
 	}
+	return 0;
 }
 
 static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options)
diff --git a/read-cache.c b/read-cache.c
index 4ac9a03..2058b7a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1360,6 +1360,7 @@ unmap:
 	munmap(mmap, mmap_size);
 	errno = EINVAL;
 	die("index file corrupt");
+	return 0;
 }
 
 int is_index_unborn(struct index_state *istate)
diff --git a/remote.c b/remote.c
index ca42a12..e22b5d4 100644
--- a/remote.c
+++ b/remote.c
@@ -655,6 +655,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 		return NULL;
 	}
 	die("Invalid refspec '%s'", refspec[i]);
+	return NULL;
 }
 
 int valid_fetch_refspec(const char *fetch_refspec_str)
diff --git a/revision.c b/revision.c
index c46cfaa..e44083a 100644
--- a/revision.c
+++ b/revision.c
@@ -255,6 +255,7 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 		return NULL;
 	}
 	die("%s is unknown object", name);
+	return NULL;
 }
 
 static int everybody_uninteresting(struct commit_list *orig)
diff --git a/setup.c b/setup.c
index ce87900..873aa2c 100644
--- a/setup.c
+++ b/setup.c
@@ -79,6 +79,7 @@ int check_filename(const char *prefix, const char *arg)
 	if (errno == ENOENT || errno == ENOTDIR)
 		return 0; /* file does not exist */
 	die_errno("failed to stat '%s'", arg);
+	return 0;
 }
 
 static void NORETURN die_verify_filename(const char *prefix, const char *arg)
diff --git a/shell.c b/shell.c
index abb8622..23ed2ec 100644
--- a/shell.c
+++ b/shell.c
@@ -215,4 +215,5 @@ int main(int argc, char **argv)
 		die("invalid command format '%s': %s", argv[2],
 		    split_cmdline_strerror(count));
 	}
+	return 0;
 }
diff --git a/submodule.c b/submodule.c
index b6dec70..e177516 100644
--- a/submodule.c
+++ b/submodule.c
@@ -245,6 +245,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
 			return RECURSE_SUBMODULES_ON_DEMAND;
 		die("bad %s argument: %s", opt, arg);
 	}
+	return 0;
 }
 
 void show_submodule_summary(FILE *f, const char *path,
diff --git a/transport.c b/transport.c
index c9c8056..5113a64 100644
--- a/transport.c
+++ b/transport.c
@@ -1131,6 +1131,7 @@ int transport_connect(struct transport *transport, const char *name,
 		return transport->connect(transport, name, exec, fd);
 	else
 		die("Operation not supported by protocol");
+	return 0;
 }
 
 int transport_disconnect(struct transport *transport)
-- 
1.7.4.4

[PATCH 3/3] Add profile feedback build to git v2

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Add a gcc profile feedback build option "profile-all" to the
main Makefile. It simply runs the test suite to generate feedback
data and the recompiles the main executables with that. The basic
structure is similar to the existing gcov code.

gcc is often able to generate better code with profile feedback
data. The training load also doesn't need to be too similar
to the actual load, it still gives benefits.

The test suite run is unfortunately quite long. It would
be good to find a suitable subset that runs faster and still
gives reasonable feedback.

For now the test suite runs single threaded (I had some
trouble running the test suite with -jX)

I tested it with git gc and git blame kernel/sched.c on a Linux
kernel tree. For gc I get about 2.7% improvement in wall clock
time by using the feedback build, for blame about 2.4%.
That's not gigantic, but not shabby either for a very small patch.

If anyone has any favourite CPU intensive git benchmarks feel
free to try them too.

I hope distributors will switch to use a feedback build in their
packages.

v2: Set NO_NORETURN variable in build
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 Makefile |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 03b4499..d00718f 100644
--- a/Makefile
+++ b/Makefile
@@ -2492,3 +2492,20 @@ cover_db: coverage-report
 
 cover_db_html: cover_db
 	cover -report html -outputdir cover_db_html cover_db
+
+### profile feedback build
+#
+.PHONY: profile-all profile-clean
+
+PROFILE_GEN_CFLAGS := $(CFLAGS) -fprofile-generate -DNO_NORETURN=1
+PROFILE_USE_CFLAGS := $(CFLAGS) -fprofile-use -fprofile-correction -DNO_NORETURN=1
+
+profile-clean:
+	$(RM) $(addsuffix *.gcda,$(object_dirs))
+	$(RM) $(addsuffix *.gcno,$(object_dirs))
+
+profile-all: profile-clean
+	$(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" all
+	$(MAKE) CFLAGS="$(PROFILE_GEN_CFLAGS)" -j1 test
+	$(MAKE) CFLAGS="$(PROFILE_USE_CFLAGS)" all
+	
-- 
1.7.4.4

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Junio C Hamano]

Andi Kleen <andi@firstfloor.org> writes:

> From: Andi Kleen <ak@linux.intel.com>
>
> Add a lot of dummy returns to silence "control flow reaches
> end of non void function" warnings with disabled noreturn.
>
> If NO_NORETURN is not disabled they will be all optimized away.

I think this is probably a bad move, given that the previous patch is a
temporary workaround until gcc 4.6 is fixed. With -Wunreachable-code on,
these will introduce noise for build without NO_NORETURN (either when
profile feedback is not used, or when profile feedback build is in use and
it no longer requires the NO_NORETURN workaround).

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Andi Kleen]

On Mon, Jun 20, 2011 at 02:17:32PM -0700, Junio C Hamano wrote:
> Andi Kleen <andi@firstfloor.org> writes:
> 
> > From: Andi Kleen <ak@linux.intel.com>
> >
> > Add a lot of dummy returns to silence "control flow reaches
> > end of non void function" warnings with disabled noreturn.
> >
> > If NO_NORETURN is not disabled they will be all optimized away.
> 
> I think this is probably a bad move, given that the previous patch is a

This is basically the patch you suggested. Do you have some other suggestion 
now?

FWIW I preferred my original minimal patch and I can go back to that one.

> temporary workaround until gcc 4.6 is fixed. With -Wunreachable-code on,

gcc mainline (and possibly 4.6.2) has it already fixed, but it's reasonable 
to assume 4.6.0 will be in use for a long time. There's nothing
"temporary" about compiler workarounds, unless you wait 10 years
or so.

> these will introduce noise for build without NO_NORETURN (either when
> profile feedback is not used, or when profile feedback build is in use and
> it no longer requires the NO_NORETURN workaround).

I fixed the noise in a followon patch. 

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think this is probably a bad move, given that the previous patch is a
> temporary workaround until gcc 4.6 is fixed. With -Wunreachable-code on,
> these will introduce noise for build without NO_NORETURN (either when
> profile feedback is not used, or when profile feedback build is in use and
> it no longer requires the NO_NORETURN workaround).

I would need to clarify with s/introduce noise/introduce more noise/; the
existing codebase is not noise-free.

But I do not see much point in making things worse, only to squelch
"reaches end of non void function" warnings that will be given under the
NO_NORETURN workaround configuration.

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Junio C Hamano]

Andi Kleen <andi@firstfloor.org> writes:

> On Mon, Jun 20, 2011 at 02:17:32PM -0700, Junio C Hamano wrote:
>> Andi Kleen <andi@firstfloor.org> writes:
>> 
>> > From: Andi Kleen <ak@linux.intel.com>
>> >
>> > Add a lot of dummy returns to silence "control flow reaches
>> > end of non void function" warnings with disabled noreturn.
>> >
>> > If NO_NORETURN is not disabled they will be all optimized away.
>> 
>> I think this is probably a bad move, given that the previous patch is a
>
> This is basically the patch you suggested. Do you have some other suggestion 
> now?

Sorry, I do not recall suggesting to add these dummy returns. The NO_NORETURN
workaround (your [1/3]) is what I remember.

>> these will introduce noise for build without NO_NORETURN (either when
>> profile feedback is not used, or when profile feedback build is in use and
>> it no longer requires the NO_NORETURN workaround).
>
> I fixed the noise in a followon patch. 

I suspect that we are talking about different warnings.

The extra unreachable returns this patch adds will introduce more
"unreachable code" warnings, which was what my message you are responding
to is about.

While I agree with you that this patch will now squelch "control flow
reaches end of non void function" warnings (which is your justification
for this patch), I am just pointing out that it is like robbing Peter to
pay Paul.

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Andi Kleen]

> I would need to clarify with s/introduce noise/introduce more noise/; the
> existing codebase is not noise-free.
> 
> But I do not see much point in making things worse, only to squelch
> "reaches end of non void function" warnings that will be given under the
> NO_NORETURN workaround configuration.

Can you please give specific guidance what I should do to make
the patchkit acceptable?

Current options are:

1) use original minimal patchkit (which had two warnings or so)
1b) use original minimal patchkit with warnings fixed
2) use global patch proposal for NO_NORETURN (= lots of warnings)
2b) use patch proposal + additional patch to fix warnings (posted here)
3) something I missed.

Which one do you prefer? If 3 I would prefer specific guidance.

Thanks,

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Andi Kleen]

> Sorry, I do not recall suggesting to add these dummy returns. The NO_NORETURN
> workaround (your [1/3]) is what I remember.

It was just a logical followup to quelch the warning storm NO_NORETURN 
caused.

> 
> >> these will introduce noise for build without NO_NORETURN (either when
> >> profile feedback is not used, or when profile feedback build is in use and
> >> it no longer requires the NO_NORETURN workaround).
> >
> > I fixed the noise in a followon patch. 
> 
> I suspect that we are talking about different warnings.
> 
> The extra unreachable returns this patch adds will introduce more
> "unreachable code" warnings, which was what my message you are responding
> to is about.

Hmm I didn't see any additional warnings from the patch, neither
in profile feedback nor in a normal build with gcc 4.5. Did I miss
something?

-Andi

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Junio C Hamano]

Andi Kleen <andi@firstfloor.org> writes:

>> I would need to clarify with s/introduce noise/introduce more noise/; the
>> existing codebase is not noise-free.
>> 
>> But I do not see much point in making things worse, only to squelch
>> "reaches end of non void function" warnings that will be given under the
>> NO_NORETURN workaround configuration.
>
> Can you please give specific guidance what I should do to make
> the patchkit acceptable?

I am inclined to apply only 1 and 3, which is what I already have on 'pu'.

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Jonathan Nieder]

Hi Andi,

Andi Kleen wrote:

>> Sorry, I do not recall suggesting to add these dummy returns. The NO_NORETURN
>> workaround (your [1/3]) is what I remember.
>
> It was just a logical followup to quelch the warning storm NO_NORETURN 
> caused.

Please remember to think for yourself. ;-)  Junio generally gives good
advice, but if you don't see the wisdom in it, that's the time to ask
questions, not blindly do a wrong thing.

In this case, since the NO_NORETURN knob is to work around a gcc bug,
wouldn't it make sense to add a -Wno-something-or-other option to
BASIC_CFLAGS or COMPAT_CFLAGS when it is set?

Hope that helps,
Jonathan

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Andi Kleen]

> I am inclined to apply only 1 and 3, which is what I already have on 'pu'.

Thanks. 

I guess should add some documentation that explains that there
are additional warnings (and some general pointers). I'll send that
in another patch.

-Andi
> 

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Andi Kleen]

> Please remember to think for yourself. ;-)  Junio generally gives good
> advice, but if you don't see the wisdom in it, that's the time to ask
> questions, not blindly do a wrong thing.

To be honest it's still not clear to me what was wrong with patch (2).

> 
> In this case, since the NO_NORETURN knob is to work around a gcc bug,
> wouldn't it make sense to add a -Wno-something-or-other option to
> BASIC_CFLAGS or COMPAT_CFLAGS when it is set?

The problem is that only relatively new gccs have options to do
fine grained control on all warnings. So this would add more complications
in the Makefile to check the compiler version

(I haven't checked if that's true for this particular warning,
but I have run into this several times in the past for others)

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Jonathan Nieder]

Andi Kleen wrote:

> To be honest it's still not clear to me what was wrong with patch (2).

Ah, ok.  From my point of view the patch was problematic since it adds
code that would be distracting to both humans and static analyzers.
For example, a person might wonder "why this return value and not
another?".  Even worse, if someone removes a die() call and introduces
a bug, we lose the benefit of the warnings you are suppressing.

>> In this case, since the NO_NORETURN knob is to work around a gcc bug,
>> wouldn't it make sense to add a -Wno-something-or-other option to
>> BASIC_CFLAGS or COMPAT_CFLAGS when it is set?
>
> The problem is that only relatively new gccs have options to do
> fine grained control on all warnings. So this would add more complications
> in the Makefile to check the compiler version

The default CFLAGS is very simple because git is supposed to be
possible to build with other compilers, too, and I wasn't suggesting
changing that.  But I suspect making NO_NORETURN assume a modern
enough gcc to trigger the bug might be ok.

Anyway, thanks for writing these patches.  I'm happy to see git get
faster.  As a side question, do you know if gcc provides a way to
print output about what profile-driven optimizations were especially
compelling, so they could help people think about how to reorganize
code to improve the non profile-driven builds, too?

Regards,
Jonathan

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Jonathan Nieder]

Jonathan Nieder wrote:

> Even worse, if someone removes a die() call and introduces
> a bug, we lose the benefit of the warnings you are suppressing.

(Scratch that part --- I don't know what I was thinking.  Will
think more carefully before hitting "send" next time.)

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Junio C Hamano]

Andi Kleen <andi@firstfloor.org> writes:

>> Please remember to think for yourself. ;-)  Junio generally gives good
>> advice, but if you don't see the wisdom in it, that's the time to ask
>> questions, not blindly do a wrong thing.
>
> To be honest it's still not clear to me what was wrong with patch (2).

For example.

    diff --git a/builtin/commit.c b/builtin/commit.c
    index 5286432..51ee2e5 100644
    --- a/builtin/commit.c
    +++ b/builtin/commit.c
    @@ -962,6 +962,7 @@ static const char *find_author_by_nickname(const char *name)
                    return strbuf_detach(&buf, NULL);
            }
            die(_("No existing author found with '%s'"), name);
    +	return NULL;
     }

When the above is applied and compiled without NO_NORETURN, the extra
return may be optimized out by the compiler as your commit log messages
said, but wouldn't it introduce a new warning:

  builtin/commit.c: In function 'find_author_by_nickname':
  builtin/commit.c:965: error: will never be executed

under -Wunreachable-code?

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Andi Kleen]

> When the above is applied and compiled without NO_NORETURN, the extra
> return may be optimized out by the compiler as your commit log messages
> said, but wouldn't it introduce a new warning:
> 
>   builtin/commit.c: In function 'find_author_by_nickname':
>   builtin/commit.c:965: error: will never be executed
> 
> under -Wunreachable-code?

It may, but that option isn't set for git?

-Andi

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Andi Kleen]

> Anyway, thanks for writing these patches.  I'm happy to see git get
> faster.  As a side question, do you know if gcc provides a way to
> print output about what profile-driven optimizations were especially
> compelling, so they could help people think about how to reorganize
> code to improve the non profile-driven builds, too?

Generally gcc has no idea how much difference an optimization makes.
It would need to run the code for that, but it doesn't.

That's generally only possible for JITs.

For some optimizations (basic block reordering) you could get
the same benefit with __builtin_expect.

But based on my own experience with __builtin_expect in other projects
I strongly recommend to not use it manually: people tend
to use it everywhere eventually and they often get it wrong.
Humans are quite bad at deciding such things. Also code behaviour
changes over time and then the annotations often become outdated.

[e.g. the kernel has a special profiler for builtin_expects --
aka unlikely -- which checks the manual annotation against
the true runtime behaviour and the failure rate of manual annotation 
is quite spectacular]

In addition there are various optimizations in gcc where I am not
aware of a manual annotation possibility (like register allocation). 
The data from profile feedback is used in quite a lot of places all
over the compiler.

-Andi

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Junio C Hamano]

Andi Kleen <andi@firstfloor.org> writes:

>> I am inclined to apply only 1 and 3, which is what I already have on 'pu'.
>
> Thanks. 

Thank _you_ for working on this.

> I guess should add some documentation that explains that there
> are additional warnings (and some general pointers). I'll send that
> in another patch.

Yup, applied. Thanks.

Re: [PATCH 2/3] Add a lot of dummy returns to avoid warnings with NO_NORETURN

[Jonathan Nieder]

Andi Kleen wrote:

> For some optimizations (basic block reordering) you could get
> the same benefit with __builtin_expect.
>
> But based on my own experience with __builtin_expect in other projects
> I strongly recommend to not use it manually:
[...]
> In addition there are various optimizations in gcc where I am not
> aware of a manual annotation possibility (like register allocation). 
> The data from profile feedback is used in quite a lot of places all
> over the compiler.

Thanks.  Alas.