nemequ / hedley

A C/C++ header to help move #ifdefs out of your code
https://nemequ.github.io/hedley/
Creative Commons Zero v1.0 Universal
774 stars 51 forks source link

NVidia/PGI `[[noreturn]] static` vs. `static _Noreturn` #53

Open ludocode opened 2 years ago

ludocode commented 2 years ago

Hi there,

The NVidia/PGI compiler has some interesting behaviour regarding noreturn placement around storage specifiers. This creates a limitation in making a wrapper macro that can be both [[noreturn]] in C++ and _Noreturn in C.

In C++ the [[noreturn]] attribute must come before storage class specifiers. Example:

[[noreturn]]
static
void fatal(const char* msg) {
    fprintf(stderr, "%s\n", msg);
    abort();
}

Many compilers including GCC g++ and NVidia nvc++ enforce this. They give an attribute error if [[noreturn]] is placed after static.

However, in C, the NVidia/PGI compiler under -Wpedantic requires that the storage specifier come first. The definition has to look like this:

static
_Noreturn
void fatal(const char* msg) {
    fprintf(stderr, "%s\n", msg);
    abort();
}

If _Noreturn is instead placed before static, the NVidia compiler throws a pedantic warning. This is the only compiler I've found that complains about this.

This creates a problem with your HEDLEY_NO_RETURN macro. Suppose I am making a header-only library that can be included in both C and C++ and I want to declare a noreturn static function. Where do I put HEDLEY_NO_RETURN? It's easy to reproduce this with your test suite. With nvc 22.5 in hedley/test/:

nvc --display_error_number -Wpedantic -Werror no-return.c

This prints:

"no-return.c", line 22: error #82: storage class is not first
  static void
  ^

1 error detected in the compilation of "no-return.c".

What do you think would be the best way to handle this? Since NVidia is the outlier here it's possible to simply silence the warning: _Pragma("diag_suppress 82") works. I don't know what else this error number might silence though. Unfortunately NVidia doesn't support push/pop for silencing warnings so I can't scope this to my own header files either. Do you think this pragma should be added to HEDLEY_NO_RETURN under these specific circumstances to silence it automatically? Or do you want to document this behaviour and have users disable the warning themselves?

Full disclosure, I ask because I am making my own portability library similar to Hedley and portable-snippets. I suppose it will be a competitor of sorts but I am hoping we can help each other out to make all of them good. (And although I haven't used anything from Hedley or portable-snippets yet, my library will also be under a public-domain-equivalent license so we could share pieces of code back and forth if you're open to it.)

Anyway I ran into this problem with my own library and was curious to see whether you handled it. Looks like you haven't yet. Your library doesn't appear to generate tests for NVidia/PGI although it has a lot of code specific to their compiler. Do you test with the NVidia compiler? (I am hoping you have some tests for it because I have a bigger problem with NVidia regarding unreachable code that I might ask about next...)

ludocode commented 2 years ago

Upon further testing it appears __attribute__((__noreturn__)) does not throw the warning under NVidia/PGI if it comes before static. So maybe the solution is to simply use that instead of _Noreturn in C. (And presumably C23 [[noreturn]] will not have this problem either.)

The below patch fixes it without having to silence any warnings. It uses the GNU-style attribute in C++ as well though which is not necessary, and I presume you will want a proper version macro check so I didn't bother making a PR.

diff --git a/hedley.h b/hedley.h
index 8a713e6..6d5a3b6 100644
--- a/hedley.h
+++ b/hedley.h
@@ -1148,6 +1148,7 @@
 #if HEDLEY_IAR_VERSION_CHECK(8,0,0)
 #  define HEDLEY_NO_RETURN __noreturn
 #elif \
+  defined(__PGI) || \
   HEDLEY_INTEL_VERSION_CHECK(13,0,0) || \
   HEDLEY_MCST_LCC_VERSION_CHECK(1,25,10)
 #  define HEDLEY_NO_RETURN __attribute__((__noreturn__))