Closed ryandesign closed 5 years ago
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
.)
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.
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.)
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.
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).
@ryandesign As I don't have a Mac, could you please check the patch proposed in #45.
@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.
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:
I'm able to fix this using: