From: Erik Faye-Lund Date: Mon, 14 Sep 2009 11:40:02 GMT Subject: Re: [PATCH 2/2] remove NORETURN from function pointers Message-ID: <40aa078e0909140440x2e189957uf66f36ff29bef302@mail.gmail.com> In-Reply-To: <20090914105750.GB9216@sigill.intra.peff.net> On Mon, Sep 14, 2009 at 12:57 PM, Jeff King 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 > 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