Add some macros to abstract marking of parameters as "not null", and use them in <cstring>

[mclow.lists]

Summary

The C standard says that calling memcpy, etc with null parameters is undefined behavior.
GCC (and clang) have attributes that allow us to mark the parameters of functions as "must not be null".

Define a mechanism to do this, and use it (for the first time) to mark the parameters of memcpy, memmove, memcmp and strncmp as "must not be null".

This gives us compile time checking for constant pointers, and hints to the code generator.

Note: This will not be a big win on systems that use glibc, because it marks the global functions ::memcpy, etc the same way. On Mac OS X, iOS, Android, FreeBSD, etc, this will make a bigger difference.

I will be adding tests as well; this post is to gather consensus that this is the right way to go.

[nlopes]

include/cstring

96

typo here.

[joerg]

I'm against doing this unconditionally. IMO it creates bugs without reasonable compensation. Just because glibc wants to hurt people doesn't mean anyone should get hurt.

[danalbert]

In D11948#221936, @joerg wrote:

I'm against doing this unconditionally. IMO it creates bugs without reasonable compensation. Just because glibc wants to hurt people doesn't mean anyone should get hurt.

+1. We don't want this in Android.

[mclow.lists]

include/cstring

96

Oops. Thanks!

[mclow.lists]

In D11948#221936, @joerg wrote:

I'm against doing this unconditionally. IMO it creates bugs without reasonable compensation. Just because glibc wants to hurt people doesn't mean anyone should get hurt.

Really? I see it as: It tells people who have UB in their code that they have a bug.

There's nothing here that "creates bugs".

[joerg]

No, it doesn't. It tells the compiler that it is free to make such assumptions. Take a step back from the standard. Can you think of any reasonable and efficient implementation of memcpy and friends, which fails for size 0? Adding the annotations (whether here or in string.h) effectively changes the behavior of the program. It is behavior people have been expecting for two decades, even when C90 said something else. This is completely different from the warning annotations. I'm just waiting for some of the bigger projects like PostgreSQL to start getting annoyed enough to introduce sane_memcpy for this.
I can't speak for Linux distributions using glibc, but I find this kind of smoking gun completely unacceptable to force unconditionally on everyone.

[mclow.lists]

In D11948#221991, @joerg wrote:

No, it doesn't. It tells the compiler that it is free to make such assumptions.

Again, I disagree. The compiler already knows it is free to make such assumptions.
(LLVM has an entire optimizer pass devoted to memcpy and friends)

[joerg]

LLVM makes the assumptions only if the prototype has them too? Attribute nonnull is certainly changing optimiser behavior with GCC...

[hfinkel]

• Original Message -----

From: "Marshall Clow via cfe-commits" <[e-mail]>
To: "mclow lists" <[e-mail]>, [e-mail], [e-mail], [e-mail]
Cc: [e-mail], [e-mail]
Sent: Tuesday, August 11, 2015 3:30:10 PM
Subject: Re: [PATCH] D11948: Add some macros to abstract marking of parameters as "not null", and use them in
<cstring>

mclow.lists added a comment.

In http://reviews.llvm.org/D11948#221991, @joerg wrote:

No, it doesn't. It tells the compiler that it is free to make such
assumptions.

Again, I disagree. The compiler already knows it is free to make such
assumptions.
(LLVM has an entire optimizer pass devoted to memcpy and friends)

I agree with Marshall. This ship has already sailed - the standard says what is says, and GCC and other compilers already take advantage of it, and thus portable code already cannot depend on the behavior of passing null pointers. Even non-portable code will be locked to specific compiler/library versions (which is already true with GCC) because, annotations aside, compilers are free to assume the standard semantics regardless. The annotations at least give users some more information on what the compiler is assuming, and so having them is better than not.

-Hal

http://reviews.llvm.org/D11948

---

cfe-commits mailing list
[e-mail]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[chandlerc]

I agree with both Marshall and Hal.

However, I'm also not opposed to platforms opting out if this if they want to. If a particular platform wants to define the behavior of null pointers passed to these functions, that seems like a reasonable thing to support here. The two platforms that seem interested in supporting null pointers here are Android and FreeBSD, so I think we should disable this for those platforms.

But I want to point out and emphasize what Hal said: the compiler can *already do this*. Please make sure that both of those platforms configure Clang and LLVM to specially handle these routines to support null pointers. You're providing a conforming extension for your platform, but you can't *just* do that in the STL, you need to do that at every layer of the toolchain.

For Linux, given the *very* long precedent of glibc and GCC behavior, I'm confident we want these annotations. I'll let folks that actually support other platforms chime up for the cases they care about.

[mclow.lists]

Fixed a typo in the patch for strncmp, and added a macro _LIBCPP_HAS_NO_NONNULL to allow people to disable these checks.

Marked them as disabled for non-clang and non-gcc compilers.

include/__config

 
 #define _LIBCPP_UNUSED __attribute__((__unused__))
 
+#define _LIBCPP_NON_NULL      __attribute__((__nonnull__))
+#define _LIBCPP_NON_NULL1(x)    __attribute__((__nonnull__(x)))
+#define _LIBCPP_NON_NULL2(x,y)    __attribute__((__nonnull__(x,y)))
+#define _LIBCPP_NON_NULL3(x,y,z)  __attribute__((__nonnull__(x,y,z)))
+
 #if !(__has_feature(cxx_defaulted_functions))
 #define _LIBCPP_HAS_NO_DEFAULTED_FUNCTIONS
 #endif  // !(__has_feature(cxx_defaulted_functions))

 
 #define _LIBCPP_UNUSED __attribute__((__unused__))
 
+#define _LIBCPP_NON_NULL      __attribute__((__nonnull__))
+#define _LIBCPP_NON_NULL1(x)    __attribute__((__nonnull__(x)))
+#define _LIBCPP_NON_NULL2(x,y)    __attribute__((__nonnull__(x,y)))
+#define _LIBCPP_NON_NULL3(x,y,z)  __attribute__((__nonnull__(x,y,z)))
+
 #if _GNUC_VER >= 407
 #define _LIBCPP_UNDERLYING_TYPE(T) __underlying_type(T)
 #define _LIBCPP_IS_LITERAL(T) __is_literal_type(T)

include/cstring

 _LIBCPP_BEGIN_NAMESPACE_STD
 
 using ::size_t;
-using ::memcpy;
-using ::memmove;
+
+// using ::memcpy;
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_NON_NULL2(1, 2)
+void* memcpy(void* __s1, const void* __s2, size_t __n) 
+{ return ::memcpy(__s1, __s2, __n); }
+
+// using ::memmove;
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_NON_NULL2(1, 2)
+void* memmove(void* __s1, const void* __s2, size_t __n)
+{ return ::memmove(__s1, __s2, __n); }
+
 using ::strcpy;
 using ::strncpy;
 using ::strcat;
 using ::strncat;
-using ::memcmp;
+
+// using ::memcmp;
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_NON_NULL2(1, 2)
+int memcmp(const void* __s1, const void* __s2, size_t __n)
+{ return ::memcmp(__s1, __s2, __n); }
+
 using ::strcmp;
-using ::strncmp;
+
+// using ::strncmp;
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_NON_NULL2(1, 2)
+int strncmp(const char* __s1, const char* __s2, size_t __n)
+{ return ::memcmp(__s1, __s2, __n); }
+
 using ::strcoll;
 using ::strxfrm;