error: '__nonnull' macro redefined

[ryandesign]

Hello, trying to build libxcrypt fails for me on macOS. I mentioned this error as part of #40 but it probably deserves its own issue:

In file included from gen-des-tables.c:49:
In file included from ./crypt-port.h:314:
./crypt.h:24:9: error: '__nonnull' macro redefined [-Werror,-Wmacro-redefined]
#define __nonnull(arg) /* nothing */
        ^
./crypt-port.h:56:9: note: previous definition is here
#define __nonnull(param) /* nothing */
        ^

I'm able to fix this using:

--- gen-crypt-h.awk.orig	2018-10-26 09:35:49.000000000 -0500
+++ gen-crypt-h.awk	2018-10-28 20:57:03.000000000 -0500
@@ -43,6 +43,7 @@
             print "#define __THROW /* nothing */"
         }
         if (!HAVE_SYS_CDEFS_NONNULL) {
+            print "#undef __nonnull"
             print "#define __nonnull(arg) /* nothing */"
         }
         print ""

[zackw]

Could you please look through your system headers and find out where __nonnull is getting defined and what it's getting defined to? I would want to make sure we don't screw something else up with that change. (I'm particularly concerned with the possibility of someone including crypt.h and then a system header that needs the original definition of __nonnull.)

[ryandesign]

Sure, it looks like on OS X 10.11 and later, /usr/include/sys/cdefs.h contains:

/* Compatibility with compilers and environments that don't support the
 * nullability feature.
 */

#if !__has_feature(nullability)
#ifndef __nullable
#define __nullable
#endif
#ifndef __nonnull
#define __nonnull
#endif
#ifndef __null_unspecified
#define __null_unspecified
#endif
#ifndef _Nullable
#define _Nullable
#endif
#ifndef _Nonnull
#define _Nonnull
#endif
#ifndef _Null_unspecified
#define _Null_unspecified
#endif
#endif

On OS X 10.10 and earlier, that header doesn't define __nonnull.

There are also several other headers in /System/Library/Frameworks that similarly #define __nonnull to empty if not defined. I don't see anywhere on any version of macOS that defines __nonnull to anything other than empty.

[zackw]

Oh, that's annoying. We're expecting this definition of __nonnull out of sys/cdefs.h:

#if __GNUC_PREREQ (3,3)
# define __nonnull(params) __attribute__ ((__nonnull__ params))
#else
# define __nonnull(params)
#endif

This means your patch will break MacOS system headers included after crypt.h, which I bet are doing things like

extern double atof (const char *__nonnull __nptr);

instead of the GNUish sort-of-equivalent

extern double atof (const char *__nptr) __nonnull ((1));

I'm tempted to address this by ripping out all of the nonnull annotations, which are a questionable optimization that have caused other problems in the past. @besser82 do you have an opinion?

(In case you don't see why our redefinition would break the system headers, we're defining it as a function-like macro, so it won't be expanded if it's used without any function-call parentheses, and when the Clang "nullability" feature isn't available, it won't be recognized as a keyword either.)

[ryandesign]

I don't know, I may not have found the whole story yet. cdefs.h is only setting __nonnull empty if nullability isn't supported. I don't know what happens when nullalbility is supported.

According to a swift blog post, nullability is supported ever since Xcode 6.3. They also mention:

Due to potential conflicts with third-party libraries, we’ve changed them in Xcode 7 to the _Nullable and _Nonnull you see here. However, for compatibility with Xcode 6.3 we’ve predefined macros __nullable and __nonnull to expand to the new names.

You're correct that the macOS system headers use _Nonnull (and still a surprising amount of __nonnull even now with Xcode 9.4.1) not as a function.

[besser82]

I'm tempted to address this by ripping out all of the nonnull annotations, which are a questionable optimization that have caused other problems in the past. @besser82 do you have an opinion?

Then let's get rid of them in v4.3.0 (upcoming release).

[besser82]

@ryandesign As I don't have a Mac, could you please check the patch proposed in #45.

[zackw]

@ryandesign Based on https://clang.llvm.org/docs/AttributeReference.html#nullability-attributes I think clang will recognize most of these identifiers as keywords when __has_feature(nullability) is true.