[PATCH 2/2] remove NORETURN from function pointers

From: Erik Faye-Lund <redacted>

Some compilers (including at least MSVC and ARM RVDS) supports
NORETURN on function declarations, but not on function pointers.

This patch makes it possible to define NORETURN for these compilers.

Signed-off-by: Erik Faye-Lund <redacted>
---

__attribute__((noreturn)) is, according to the GCC documentation, about
two things: code generation (performance, really) and warnings.

On the warnings-side, we need to keep the code warning-free for
compilers who doesn't support noreturn anyway, so hiding potential
warnings through this mechanism is probably not a good idea in the
first place.

We still want the performance-side of it, though. However, the only
place this really makes a difference is to die and it's variants, since
they can potentially be called many times (or so it seems from the
compiler's point of view without a noreturn declaration).

The function pointers are only called once we're already exiting, and
they have only one potential call-site.

I hope this all makes sense ;)

 git-compat-util.h |    2 +-
 usage.c           |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 5876d91..15fe08e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -183,7 +183,7 @@ extern NORETURN void die_errno(const char *err, ...) __attribute__((format (prin
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
-extern void set_die_routine(void (*routine)(const char *err, va_list params) NORETURN);
+extern void set_die_routine(void (*routine)(const char *err, va_list params));
 
 extern int prefixcmp(const char *str, const char *prefix);
 extern time_t tm_to_time_t(const struct tm *tm);

diff --git a/usage.c b/usage.c
index b6aea45..18d7f43 100644
--- a/usage.c
+++ b/usage.c

@@ -36,12 +36,12 @@ static void warn_builtin(const char *warn, va_list params)
 
 /* If we are in a dlopen()ed .so write to a global variable would segfault
  * (ugh), so keep things static. */
-static void (*usage_routine)(const char *err) NORETURN = usage_builtin;
-static void (*die_routine)(const char *err, va_list params) NORETURN = die_builtin;
+static void (*usage_routine)(const char *err) = usage_builtin;
+static void (*die_routine)(const char *err, va_list params) = die_builtin;
 static void (*error_routine)(const char *err, va_list params) = error_builtin;
 static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
 
-void set_die_routine(void (*routine)(const char *err, va_list params) NORETURN)
+void set_die_routine(void (*routine)(const char *err, va_list params))
 {
 	die_routine = routine;
 }

-- 
1.6.4.msysgit.0.16.gd92d4.dirty

On Mon, Sep 14, 2009 at 12:16:10PM +0200, Erik Faye-Lund wrote:

From: Erik Faye-Lund <redacted>

Some compilers (including at least MSVC and ARM RVDS) supports
NORETURN on function declarations, but not on function pointers.

This patch makes it possible to define NORETURN for these compilers.

Signed-off-by: Erik Faye-Lund <redacted>

I didn't see a patch 1/2, so maybe it impacts this in some way, but by
itself, I don't think this patch is a good idea. See below.

---

__attribute__((noreturn)) is, according to the GCC documentation, about
two things: code generation (performance, really) and warnings.

On the warnings-side, we need to keep the code warning-free for
compilers who doesn't support noreturn anyway, so hiding potential
warnings through this mechanism is probably not a good idea in the
first place.

[Your justification text would almost certainly be useful as part of the
commit message itself, and should go there.]

Unfortunately, this patch _introduces_ warnings when running with gcc,
as it now thinks those function pointers return (which means it thinks
die() returns). So simply removing the NORETURN is not a good idea.

If I understand you correctly, the problem is that there are actually
three classes of compilers:

  1. Ones which understand some NORETURN syntax for both functions and
     function pointers, and correctly trace returns through both (e.g.,
     gcc).

  2. Ones which understand some NORETURN syntax for just functions, and
     complain about it on function pointers. We currently turn off
     NORETURN for these compilers (from your commit message, MSVC,
     for example).

  3. Ones which have no concept of NORETURN at all.

I think the right solution to turn on NORETURN for (2) is to split it
into two cases: NORETURN and NORETURN_PTR. Gcc platforms can define both
identically, platforms under (2) above can define NORETURN_PTR as empty,
and (3) will keep both off.

I do have to wonder, though. What do compilers that fall under (2) do
with calls to function pointers from NORETURN functions? Do they assume
they don't return, or that they do? Or do they not check that NORETURN
functions actually don't return?

I.e., what does this program do under MSVC:

#include <stdlib.h>
void (*exit_fun)(int) = exit;
static void die(void) __attribute__((__noreturn__));
static void die(void) { exit_fun(1); }
int main(void) { die(); }

In gcc, it rightly complains:

  foo.c: In function ‘die’:
  foo.c:4: warning: ‘noreturn’ function does return

-Peff

On Mon, Sep 14, 2009 at 12:57 PM, Jeff King [off-list ref] wrote:

I didn't see a patch 1/2, so maybe it impacts this in some way, but by
itself, I don't think this patch is a good idea. See below.

That's odd. It's listed in my git-folder on GMail as sent to the
mailing-list, but I can't find it in any of the list-archives. They
were both sent through the same instance of "git send-email". I guess
I'll just re-send it. It shouldn't affect this patch directly, though.

The patch basically moved the NORETURN before the function-name, as
this is a placement where more compilers supports
declaration-specifications.

quoted

---

__attribute__((noreturn)) is, according to the GCC documentation, about
two things: code generation (performance, really) and warnings.

On the warnings-side, we need to keep the code warning-free for
compilers who doesn't support noreturn anyway, so hiding potential
warnings through this mechanism is probably not a good idea in the
first place.

[Your justification text would almost certainly be useful as part of the
commit message itself, and should go there.]

OK, I'll include it in the next round.

Unfortunately, this patch _introduces_ warnings when running with gcc,
as it now thinks those function pointers return (which means it thinks
die() returns). So simply removing the NORETURN is not a good idea.

Yeah, this is unacceptable. I can't believe I missed this - sorry about that!

If I understand you correctly, the problem is that there are actually
three classes of compilers:

 1. Ones which understand some NORETURN syntax for both functions and
    function pointers, and correctly trace returns through both (e.g.,
    gcc).

 2. Ones which understand some NORETURN syntax for just functions, and
    complain about it on function pointers. We currently turn off
    NORETURN for these compilers (from your commit message, MSVC,
    for example).

 3. Ones which have no concept of NORETURN at all.

I think the right solution to turn on NORETURN for (2) is to split it
into two cases: NORETURN and NORETURN_PTR. Gcc platforms can define both
identically, platforms under (2) above can define NORETURN_PTR as empty,
and (3) will keep both off.

Yeah, this could work. I initially suggested doing this, but Junio
suggested removing NORETURN all together. I didn't think that was a
good idea for die() etc, thus this patch.

I do have to wonder, though. What do compilers that fall under (2) do
with calls to function pointers from NORETURN functions? Do they assume
they don't return, or that they do? Or do they not check that NORETURN
functions actually don't return?

I.e., what does this program do under MSVC:

#include <stdlib.h>
void (*exit_fun)(int) = exit;
static void die(void) __attribute__((__noreturn__));
static void die(void) { exit_fun(1); }
int main(void) { die(); }

Well, it fails to compile ;)

If your change it around this way (which is basically what 1/2 + a
separate patch that is cooking in msysgit for a litte while longer is
supposed to do), it does compiles without warnings even on the highest
warning level:

-static void die(void) __attribute__((__noreturn__));
+static void __declspec(noreturn) die(void);

In gcc, it rightly complains:

 foo.c: In function ‘die’:
 foo.c:4: warning: ‘noreturn’ function does return

Yeah. So we need a portable (enough) way of showing it that it does
die. How about abort()?

-static void die(void) { exit_fun(1); }
+static void die(void) { exit_fun(1); abort(); }

Adding abort() makes the warning go away here, at least. And reaching
this point is an error anyway, it means that one of the functions
passed to set_die_routine() does not hold up it's guarantees. Your
suggestion (double defines) would make this a compile-time warning
instead of a run-time error, which I find much more elegant. However,
it's questionable how much this means in reality - there's only two
call-sites for set_die_routine() ATM. Do we expect it to grow a lot?

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

On Mon, Sep 14, 2009 at 1:40 PM, Erik Faye-Lund
[off-list ref] wrote:

On Mon, Sep 14, 2009 at 12:57 PM, Jeff King [off-list ref] wrote:

quoted

I didn't see a patch 1/2, so maybe it impacts this in some way, but by
itself, I don't think this patch is a good idea. See below.

That's odd. It's listed in my git-folder on GMail as sent to the
mailing-list, but I can't find it in any of the list-archives. They
were both sent through the same instance of "git send-email". I guess
I'll just re-send it.

OK, I think I figured out why: For some reason, the From-field of the
mail had gotten changed from kus...e@gmail.com to
kus...e@googlemail.com, and that's not the address I'm subsribed to
this list as. I hope whoever manages the list is able to remove my
duplicates from the moderation-queue or something ;)

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

On Mon, Sep 14, 2009 at 01:40:02PM +0200, Erik Faye-Lund wrote:

quoted

I didn't see a patch 1/2, so maybe it impacts this in some way, but by
itself, I don't think this patch is a good idea. See below.

That's odd. It's listed in my git-folder on GMail as sent to the
mailing-list, but I can't find it in any of the list-archives. They
were both sent through the same instance of "git send-email". I guess
I'll just re-send it. It shouldn't affect this patch directly, though.

Possibly it got swallowed by vger's list filter. The taboo list is here:

  http://vger.kernel.org/majordomo-taboos.txt

quoted

I think the right solution to turn on NORETURN for (2) is to split it
into two cases: NORETURN and NORETURN_PTR. Gcc platforms can define both
identically, platforms under (2) above can define NORETURN_PTR as empty,
and (3) will keep both off.

Yeah, this could work. I initially suggested doing this, but Junio
suggested removing NORETURN all together. I didn't think that was a
good idea for die() etc, thus this patch.

Doing it this way would keep warnings on compilers that could use them.
I am generally of the opinion that since most development happens on
gcc, it is "good enough" to let gcc warnings help us find broken code,
and then those fixes will be available to users of less-capable
compilers. And we don't have to bend over backwards in the code with
little hacks to trick all compilers into not issuing a warning (like
calling abort() after something we already know is going to exit).

The downside of that attitude is that code that is not exercised by gcc
builds does not get the benefit. And in the case of MSVC, there is
probably quite a bit of code in #ifdef's that gcc will never even parse.
So maybe a hack like abort() is worthwhile in this case.

quoted

#include <stdlib.h>
void (*exit_fun)(int) = exit;
static void die(void) __attribute__((__noreturn__));
static void die(void) { exit_fun(1); }
int main(void) { die(); }

Well, it fails to compile ;)

If your change it around this way (which is basically what 1/2 + a
separate patch that is cooking in msysgit for a litte while longer is
supposed to do), it does compiles without warnings even on the highest
warning level:

-static void die(void) __attribute__((__noreturn__));
+static void __declspec(noreturn) die(void);

Hmm. So either it doesn't bother checking that noreturn functions don't
return, or it assumes that a function pointer may exit.  Interesting,
but I guess it doesn't change the main point too much: it's not as
strict as gcc's checking.

Yeah. So we need a portable (enough) way of showing it that it does
die. How about abort()?

-static void die(void) { exit_fun(1); }
+static void die(void) { exit_fun(1); abort(); }

Adding abort() makes the warning go away here, at least. And reaching
this point is an error anyway, it means that one of the functions
passed to set_die_routine() does not hold up it's guarantees. Your
suggestion (double defines) would make this a compile-time warning
instead of a run-time error, which I find much more elegant. However,
it's questionable how much this means in reality - there's only two
call-sites for set_die_routine() ATM. Do we expect it to grow a lot?

No, I don't think we expect it to grow. Mostly this is about documenting
our assumptions so that gcc can do the right thing in making die() a
noreturn function, which is what we actually care about. We would notice
very quickly, I think, if a die() handler did not actually exit.

I think I am fine doing it either way. The NORETURN_PTR thing is a bit
more elegant to me, but that is maybe just my gcc snobiness. We
shouldn't have to change our code to accomodate MSVC's crappy noreturn
handling. ;)

-Peff

On Mon, Sep 14, 2009 at 01:56:29PM +0200, Erik Faye-Lund wrote:

quoted

That's odd. It's listed in my git-folder on GMail as sent to the
mailing-list, but I can't find it in any of the list-archives. They
were both sent through the same instance of "git send-email". I guess
I'll just re-send it.

OK, I think I figured out why: For some reason, the From-field of the
mail had gotten changed from kus...e@gmail.com to
kus...e@googlemail.com, and that's not the address I'm subsribed to
this list as. I hope whoever manages the list is able to remove my
duplicates from the moderation-queue or something ;)

I doubt that is it; you don't have to be subscribed to post.

-Peff

Compiling the following code gives a warning about unreachable code,
so it's clear that msvc doesn't simply ignore the directive. I'm not
saying that anyone suggested otherwise, I just wanted to know for
sure.

#include <stdio.h>
#include <stdlib.h>
void (*exit_fun)(int) = exit;
void __declspec(noreturn) die(void);
void die(void) { exit_fun(1); }
int main(void) { printf("hello!\n"); die(); printf("world!\n"); }

On Mon, Sep 14, 2009 at 2:03 PM, Jeff King [off-list ref] wrote:

I think I am fine doing it either way. The NORETURN_PTR thing is a bit
more elegant to me, but that is maybe just my gcc snobiness. We
shouldn't have to change our code to accomodate MSVC's crappy noreturn
handling. ;)

First of all, MSVC is not the only compiler that behaves this way. In
fact, GCC the only compiler I've found that behaves this way (but I
must admit, I only tested 4 different compilers, one of which (Comeau)
does not support noreturn at all AFAICT). That behavior might be
crappy, but it's not "MSVC's crappy noreturn handling" - it's
"non-GCC's crappy noreturn handling" :P

The arguments against each solution I see are these:
- abort() gives a run-time error instead of a compile-time warning, so
breakage is trickier to detect (on GCC, which seems to be the target
compiler for the vast majority of git-developers).
- NORETURN_PTR might be bit big of a hammer for a small problem, as it
"pollutes" the whole git source-tree instead of just usage.c.

Anyway, I don't care much what solution we pick. Either should work,
and if someone has strong preference, I'm OK with it.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

On Mon, Sep 14, 2009 at 02:32:38PM +0200, Erik Faye-Lund wrote:

Compiling the following code gives a warning about unreachable code,
so it's clear that msvc doesn't simply ignore the directive. I'm not
saying that anyone suggested otherwise, I just wanted to know for
sure.

#include <stdio.h>
#include <stdlib.h>
void (*exit_fun)(int) = exit;
void __declspec(noreturn) die(void);
void die(void) { exit_fun(1); }
int main(void) { printf("hello!\n"); die(); printf("world!\n"); }

Right. What I'm guessing is that it sees the noreturn on die(), but then
doesn't actually look inside die to confirm that the noreturn is upheld.
You could test that with:

#include <stdlib.h>
void __declspec(noreturn) die(void);
void die(void) { }
int main(void) { die(); }

If it doesn't complain, then I am right. If it does, then it is
something magic with the function pointer (presumably it decides that
the function pointer could exit, so it stops the analysis and gives your
code the benefit of the doubt).

First of all, MSVC is not the only compiler that behaves this way. In
fact, GCC the only compiler I've found that behaves this way (but I
must admit, I only tested 4 different compilers, one of which (Comeau)
does not support noreturn at all AFAICT). That behavior might be
crappy, but it's not "MSVC's crappy noreturn handling" - it's
"non-GCC's crappy noreturn handling" :P

Well, OK, I'll accept that. ;)

The arguments against each solution I see are these:
- abort() gives a run-time error instead of a compile-time warning, so
breakage is trickier to detect (on GCC, which seems to be the target
compiler for the vast majority of git-developers).
- NORETURN_PTR might be bit big of a hammer for a small problem, as it
"pollutes" the whole git source-tree instead of just usage.c.

I don't know that NORETURN_PTR pollutes the whole source-tree. At least
no more than NORETURN does. The only functions that will need it are
these two function pointers.

But I think your analysis is generally correct. It's not going to make a
big difference which is chosen.

-Peff

On Mon, Sep 14, 2009 at 2:44 PM, Jeff King [off-list ref] wrote:

Right. What I'm guessing is that it sees the noreturn on die(), but then
doesn't actually look inside die to confirm that the noreturn is upheld.
You could test that with:

#include <stdlib.h>
void __declspec(noreturn) die(void);
void die(void) { }
int main(void) { die(); }

If it doesn't complain, then I am right. If it does, then it is
something magic with the function pointer (presumably it decides that
the function pointer could exit, so it stops the analysis and gives your
code the benefit of the doubt).

It seems that you are right. It doesn't complain here.

quoted

The arguments against each solution I see are these:
- abort() gives a run-time error instead of a compile-time warning, so
breakage is trickier to detect (on GCC, which seems to be the target
compiler for the vast majority of git-developers).
- NORETURN_PTR might be bit big of a hammer for a small problem, as it
"pollutes" the whole git source-tree instead of just usage.c.

I don't know that NORETURN_PTR pollutes the whole source-tree. At least
no more than NORETURN does. The only functions that will need it are
these two function pointers.

Well, it does pollute the global name space, and it's "noise" when
reading the source code. But those are really very unimportant points
IMO.

And sure, that "pollution/noise" is there for the NORETURN case anyway
for a functional/practical reason, so you might argue that it
justifies the presence in the NORETURN_PTR case.

I'll wait a day or so and see if anyone else has any insight. If not,
I'll go ahead and re-post a version based on the
NORETURN_PTR-solution.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Erik Faye-Lund schrieb:

Compiling the following code gives a warning about unreachable code,
so it's clear that msvc doesn't simply ignore the directive. I'm not
saying that anyone suggested otherwise, I just wanted to know for
sure.

#include <stdio.h>
#include <stdlib.h>
void (*exit_fun)(int) = exit;
void __declspec(noreturn) die(void);
void die(void) { exit_fun(1); }
int main(void) { printf("hello!\n"); die(); printf("world!\n"); }

In order to countermand any clever optimizations you should make it

-void (*exit_fun)(int) = exit;
+extern void (*exit_fun)(int);

(of course, this fails to link). But if this results in only *one* warning
(that the printf() call is unreachable), then I wouldn't bother with this
problem anymore, because you really should also have been warned that a
__declspec(noreturn) function actually does return.

-- Hannes

On Mon, Sep 14, 2009 at 3:09 PM, Johannes Sixt [off-list ref] wrote:

Erik Faye-Lund schrieb:

quoted

Compiling the following code gives a warning about unreachable code,
so it's clear that msvc doesn't simply ignore the directive. I'm not
saying that anyone suggested otherwise, I just wanted to know for
sure.

#include <stdio.h>
#include <stdlib.h>
void (*exit_fun)(int) = exit;
void __declspec(noreturn) die(void);
void die(void) { exit_fun(1); }
int main(void) { printf("hello!\n"); die(); printf("world!\n"); }

In order to countermand any clever optimizations you should make it

-void (*exit_fun)(int) = exit;
+extern void (*exit_fun)(int);

(of course, this fails to link). But if this results in only *one* warning
(that the printf() call is unreachable), then I wouldn't bother with this
problem anymore, because you really should also have been warned that a
__declspec(noreturn) function actually does return.

Ah, good point. Still gives the warning (as well as the linker-error), though.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Erik Faye-Lund schrieb:

On Mon, Sep 14, 2009 at 3:09 PM, Johannes Sixt [off-list ref] wrote:

quoted

Erik Faye-Lund schrieb:

quoted

Compiling the following code gives a warning about unreachable code,
so it's clear that msvc doesn't simply ignore the directive. I'm not
saying that anyone suggested otherwise, I just wanted to know for
sure.

#include <stdio.h>
#include <stdlib.h>
void (*exit_fun)(int) = exit;
void __declspec(noreturn) die(void);
void die(void) { exit_fun(1); }
int main(void) { printf("hello!\n"); die(); printf("world!\n"); }

In order to countermand any clever optimizations you should make it

-void (*exit_fun)(int) = exit;
+extern void (*exit_fun)(int);

(of course, this fails to link). But if this results in only *one* warning
(that the printf() call is unreachable), then I wouldn't bother with this
problem anymore, because you really should also have been warned that a
__declspec(noreturn) function actually does return.

Ah, good point. Still gives the warning (as well as the linker-error), though.

"The" warning? Not "the two" warnings? Then I suggest to stop here; MSVC
is only half-competent with regards to noreturn.

-- Hannes

On Mon, Sep 14, 2009 at 3:19 PM, Johannes Sixt [off-list ref] wrote:

"The" warning? Not "the two" warnings? Then I suggest to stop here; MSVC
is only half-competent with regards to noreturn.

There was only one warning in this regard on MSVC - the one about
unreachable code. And yes, MSVC is only half-competent, but it seems
it's competence is in the half that matters in our case.

Do you suggest to stop the patch-series, or to stop the testing? I'd
prefer having NORETURN for die() etc in MSVC-builds, as it allows the
compiler to generate better code. I'm prefectly fine about not having
NORETURN for the function pointers. GCC should be competent enough to
catch (very theoretical) errors, and we get the nice speed-improvement
on MSVC. I don't see the down-side.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Erik Faye-Lund schrieb:

On Mon, Sep 14, 2009 at 3:19 PM, Johannes Sixt [off-list ref] wrote:

quoted

"The" warning? Not "the two" warnings? Then I suggest to stop here; MSVC
is only half-competent with regards to noreturn.

There was only one warning in this regard on MSVC - the one about
unreachable code. And yes, MSVC is only half-competent, but it seems
it's competence is in the half that matters in our case.

Do you suggest to stop the patch-series, or to stop the testing?

My suggestion was about stopping the patch series.

But thinking a bit more about it, I can imagine that there are calls to
die() that, if it is not marked noreturn, could trigger other warnings
with MSVC. That would be annoying, and it's better to mark it noreturn.

So I withdraw my suggestion to stop :-)

-- Hannes