Making _Noreturn a no-op in < Clang 16?

Making _Noreturn a no-op in < Clang 16?

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Hi all,

Over on bug-gawk, we ended up finding that Clang was miscompiling certain
expressions involving _Noreturn. This is fixed in Clang's git repo but not
in any released version. It should be in 16.0.

Paul suggested [0] that gnulib ought to #define _Noreturn to blank
for known-broken Clang versions, especially given finding such
broken patterns isn't easy.

This seems like a reasonable path forward. I'm concerned about
gnulib versions in various released bits of GNU software (diffutils
just got a release, for example) without this workaround in.

What do you think?

[0] https://lists.gnu.org/archive/html/bug-gawk/2023-01/msg00004.html

Best,
sam

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: Making _Noreturn a no-op in < Clang 16?

[Paul Eggert]

The problem we found in Gawk was that this sort of function call:

     (b ? f : g) (x)

is mishandled by Clang < 16 when one function is _Noreturn and the other 
isn't, in that Clang mistakenly treats the call as if both functions are 
_Noreturn.

I expect this sort of issue to be reasonably rare in practical C code, 
as most people don't write code like the above, and when they do then 
typically F is _Noreturn if and only if G is also _Noreturn. So I've 
held off on doing the more-drastic "#define _Noreturn /*empty*/" for 
Clang < 16 in Gnulib, as my guess has been that the advantages of 
enabling _Noreturn on Clang < 16 are greater than the disadvantages 
given the rarity of situations like the above.

Of course I could be wrong....

Re: Making _Noreturn a no-op in < Clang 16?

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]



> On 19 Jan 2023, at 04:17, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
> The problem we found in Gawk was that this sort of function call:
> 
>    (b ? f : g) (x)
> 
> is mishandled by Clang < 16 when one function is _Noreturn and the other isn't, in that Clang mistakenly treats the call as if both functions are _Noreturn.
> 
> I expect this sort of issue to be reasonably rare in practical C code, as most people don't write code like the above, and when they do then typically F is _Noreturn if and only if G is also _Noreturn. So I've held off on doing the more-drastic "#define _Noreturn /*empty*/" for Clang < 16 in Gnulib, as my guess has been that the advantages of enabling _Noreturn on Clang < 16 are greater than the disadvantages given the rarity of situations like the above.
> 

I don't have any sort of statistics on this either. _Noreturn is pretty much just an optimisation (and I'm not convinced that it's _needed_ in a lot of cases, rather just a useful hint). I'd rather
a correct result than a quicker one. But then again, you could argue that given nobody noticed for at least 13 years (including with several Linux distros building gawk with Clang), it's
not a big deal in the wild.

(err() is noreturn anyway so I'd expect the compiler to be able to deduce it.)

Up to you. I think the conservative option is to do it, but if you feel uncertain, we can probably just leave the matter. Is there any precedent wrt
handling miscompilations for actively supported compilers in gnulib and such?

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: Making _Noreturn a no-op in < Clang 16?

[Paul Eggert]

On 1/19/23 12:44, Sam James wrote:
> _Noreturn is pretty much just an optimisation (and I'm not convinced 
> that it's _needed_  in a lot of cases, rather just a useful hint).

_Noreturn is not just an optimization: it's also useful for static 
checking. For example:

   int
   f (int x)
   {
      if (x < INT_MAX)
        return x + 1;
      error (1, 0, "x is too large");
   }

Since error is _Noreturn the compiler knows not to warn that F might 
return garbage. It's useful to suppress false alarms, even when Clang is 
the compiler.

 > Is there any precedent wrt
 > handling miscompilations for actively supported compilers in gnulib 
and such?

We've run into them before; I don't know of a list of instances. 
Generally speaking if the workaround is easy and harmless we can install 
it, otherwise we tell users to get a working compiler.

Re: Making _Noreturn a no-op in < Clang 16?

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]



> On 19 Jan 2023, at 21:20, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
> On 1/19/23 12:44, Sam James wrote:
>> _Noreturn is pretty much just an optimisation (and I'm not convinced that it's _needed_  in a lot of cases, rather just a useful hint).
> 
> _Noreturn is not just an optimization: it's also useful for static checking. For example:
> 
>  int
>  f (int x)
>  {
>     if (x < INT_MAX)
>       return x + 1;
>     error (1, 0, "x is too large");
>  }
> 
> Since error is _Noreturn the compiler knows not to warn that F might return garbage. It's useful to suppress false alarms, even when Clang is the compiler.
> 

Right, I just meant that we don't tend to care about quieting warnings with older compilers,
and it's not useful from a static analysis perspective here either given that older Clangs can't be trusted.

It is of course useful as an attribute in general. I don't think either of these things are really
a downside to committing the workaround here. If we get folks who get build failures with extra warnings
enabled, we can tell them to upgrade their compiler.

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: Making _Noreturn a no-op in < Clang 16?

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

On 1/19/23 13:30, Sam James wrote:
> Right, I just meant that we don't tend to care about quieting warnings with older compilers,
> and it's not useful from a static analysis perspective here either given that older Clangs can't be trusted.
> 
> It is of course useful as an attribute in general. I don't think either of these things are really
> a downside to committing the workaround here. If we get folks who get build failures with extra warnings
> enabled, we can tell them to upgrade their compiler.

But clang 16 isn't out yet, so we can't reasonably tell people to upgrade.

And even if it clang 16 were out, I can't reasonably tell all Emacs 
developers to switch to it right away. Many of them are using Apple's 
compiler and will upgrade whenever. Plain './configure; make' on a 
bleeding-edge Emacs built from Git with Clang 15 would generate 270 
false alarms if we simply did "#define _Noreturn /**/", and I expect 
many Emacs developers would be annoyed by that (or would stop paying 
attention to any correct diagnostics mixed in with the flood of false 
positives).

With that in mind, how about the attached Gnulib patch? (I haven't 
installed it.) The basic idea is to "#define _Noreturn /**/" on buggy 
clangs if a cautious builder compiles with -D_GL_WORK_AROUND_LLVM_BUG_5979.

[-- Attachment #2: 0001-snippet-_Noreturn-work-around-Clang-_Noreturn-bug.patch --]
[-- Type: text/x-patch, Size: 3442 bytes --]

From 9d3f533586e07eba2c094501c83a22b561cc755c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 19 Jan 2023 19:39:03 -0800
Subject: [PROPOSED] snippet/_Noreturn: work around Clang _Noreturn bug
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a bigger-hammer workaround for the clang _Noreturn issue
fix for dfa.c on 2023-01-01.  Unfortunately, it causes 270
-Wreturn-type and -Wsometimes-uninitialized warnings when building
bleeding-edge GNU Emacs from Git on Fedora 37 with plain
‘./configure && make’.  So the workaround is enabled only if you
compile with -D_GL_WORK_AROUND_LLVM_BUG_59792.
* lib/_Noreturn.h (_Noreturn):
* m4/gnulib-common.m4 (gl_COMMON_BODY):
#define _Noreturn to be empty if it is Clang 15 or earlier,
and if _GL_WORK_AROUND_LLVM_BUG_59792 is defined.
---
 ChangeLog           | 14 ++++++++++++++
 lib/_Noreturn.h     |  5 +++++
 m4/gnulib-common.m4 |  5 +++++
 3 files changed, 24 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index ddd9e2b1aa..5d0c8f7cd7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2023-01-19  Paul Eggert  <eggert@cs.ucla.edu>
+
+	snippet/_Noreturn: work around Clang _Noreturn bug
+	This is a bigger-hammer workaround for the clang _Noreturn issue
+	fix for dfa.c on 2023-01-01.  Unfortunately, it causes 270
+	-Wreturn-type and -Wsometimes-uninitialized warnings when building
+	bleeding-edge GNU Emacs from Git on Fedora 37 with plain
+	‘./configure && make’.  So the workaround is enabled only if you
+	compile with -D_GL_WORK_AROUND_LLVM_BUG_59792.
+	* lib/_Noreturn.h (_Noreturn):
+	* m4/gnulib-common.m4 (gl_COMMON_BODY):
+	#define _Noreturn to be empty if it is Clang 15 or earlier,
+	and if _GL_WORK_AROUND_LLVM_BUG_59792 is defined.
+
 2023-01-19  Bruno Haible  <bruno@clisp.org>
 
 	Fix warnings for functions introduced in Android API level 34.
diff --git a/lib/_Noreturn.h b/lib/_Noreturn.h
index fa15b1b25e..6ecea98b54 100644
--- a/lib/_Noreturn.h
+++ b/lib/_Noreturn.h
@@ -26,6 +26,11 @@
        AIX system header files and several gnulib header files use precisely
        this syntax with 'extern'.  */
 #  define _Noreturn [[noreturn]]
+# elif (defined __clang__ && __clang_major__ < 16 \
+        && defined _GL_WORK_AROUND_LLVM_BUG_59792)
+   /* Compile with -D_GL_WORK_AROUND_LLVM_BUG_59792 to work around
+      that rare LLVM bug, though you may get many false-alarm warnings.  */
+#  define _Noreturn
 # elif ((!defined __cplusplus || defined __clang__) \
         && (201112 <= (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) \
             || (!defined __STRICT_ANSI__ \
diff --git a/m4/gnulib-common.m4 b/m4/gnulib-common.m4
index 2db3376b01..fa814222ce 100644
--- a/m4/gnulib-common.m4
+++ b/m4/gnulib-common.m4
@@ -38,6 +38,11 @@ AC_DEFUN([gl_COMMON_BODY], [
        AIX system header files and several gnulib header files use precisely
        this syntax with 'extern'.  */
 #  define _Noreturn [[noreturn]]
+# elif (defined __clang__ && __clang_major__ < 16 \
+        && defined _GL_WORK_AROUND_LLVM_BUG_59792)
+   /* Compile with -D_GL_WORK_AROUND_LLVM_BUG_59792 to work around
+      that rare LLVM bug, though you may get many false-alarm warnings.  */
+#  define _Noreturn
 # elif ((!defined __cplusplus || defined __clang__) \
         && (201112 <= (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) \
             || (!defined __STRICT_ANSI__ \
-- 
2.39.0

Re: Making _Noreturn a no-op in < Clang 16?

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]



> On 20 Jan 2023, at 03:40, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
> On 1/19/23 13:30, Sam James wrote:
>> Right, I just meant that we don't tend to care about quieting warnings with older compilers,
>> and it's not useful from a static analysis perspective here either given that older Clangs can't be trusted.
>> It is of course useful as an attribute in general. I don't think either of these things are really
>> a downside to committing the workaround here. If we get folks who get build failures with extra warnings
>> enabled, we can tell them to upgrade their compiler.
> 
> But clang 16 isn't out yet, so we can't reasonably tell people to upgrade.
> 
> And even if it clang 16 were out, I can't reasonably tell all Emacs developers to switch to it right away. Many of them are using Apple's compiler and will upgrade whenever. Plain './configure; make' on a bleeding-edge Emacs built from Git with Clang 15 would generate 270 false alarms if we simply did "#define _Noreturn /**/", and I expect many Emacs developers would be annoyed by that (or would stop paying attention to any correct diagnostics mixed in with the flood of false positives).
> 

I take your point and it's fair enough. Thanks for hashing it out with me.

> With that in mind, how about the attached Gnulib patch? (I haven't installed it.) The basic idea is to "#define _Noreturn /**/" on buggy clangs if a cautious builder compiles with -D_GL_WORK_AROUND_LLVM_BUG_5979.<0001-snippet-_Noreturn-work-around-Clang-_Noreturn-bug.patch>

If it's not too much of a hassle, this works for me, because at least we advertise the problem a bit, and we can tell distros
to set -D_... if they're stuck on older Clang for a bit.

Cheers Paul.

Best,
sam

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: Making _Noreturn a no-op in < Clang 16?

[Paul Eggert]

On 2023-01-19 20:20, Sam James wrote:
> If it's not too much of a hassle, this works for me, because at least we advertise the problem a bit, and we can tell distros
> to set -D_... if they're stuck on older Clang for a bit.

Thanks, I installed that into Gnulib and I expect it'll percolate into 
apps in due course.

Re: Making _Noreturn a no-op in < Clang 16?

[Bruno Haible]

Paul Eggert wrote:
> Thanks, I installed that into Gnulib and I expect it'll percolate into 
> apps in due course.

I propagated it also to the two stable branches of Gnulib [1][2].

Bruno

[1] https://lists.gnu.org/archive/html/bug-gnulib/2023-01/msg00001.html
[2] https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=heads