Re: [PATCH 2/2] remove NORETURN from function pointers

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Mon, Sep 14, 2009 at 12:57 PM, Jeff King <peff@peff.net> 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.

>> ---
>>
>> __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