[PATCH] D34294: Rework libcxx strerror_r handling.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL308528: Rework libcxx strerror_r handling. (authored by
jyknight).
Repository:
rL LLVM
https://reviews.llvm.org/D34294
Files:
libcxx/trunk/src/system_error.cpp
Index: libcxx/trunk/src/system_error.cpp
===
--- libcxx/trunk/src/system_error.cpp
+++ libcxx/trunk/src/system_error.cpp
@@ -73,39 +73,59 @@
std::snprintf(buffer, strerror_buff_size, "unknown error %d", ev);
return string(buffer);
}
-#elif defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC) &&
\
-(!defined(__ANDROID__) || __ANDROID_API__ >= 23)
-// GNU Extended version
-string do_strerror_r(int ev) {
-char buffer[strerror_buff_size];
-char* ret = ::strerror_r(ev, buffer, strerror_buff_size);
-return string(ret);
-}
#else
-// POSIX version
+
+// Only one of the two following functions will be used, depending on
+// the return type of strerror_r:
+
+// For the GNU variant, a char* return value:
+__attribute__((unused)) const char *
+handle_strerror_r_return(char *strerror_return, char *buffer) {
+ // GNU always returns a string pointer in its return value. The
+ // string might point to either the input buffer, or a static
+ // buffer, but we don't care which.
+ return strerror_return;
+}
+
+// For the POSIX variant: an int return value.
+__attribute__((unused)) const char *
+handle_strerror_r_return(int strerror_return, char *buffer) {
+ // The POSIX variant either:
+ // - fills in the provided buffer and returns 0
+ // - returns a positive error value, or
+ // - returns -1 and fills in errno with an error value.
+ if (strerror_return == 0)
+return buffer;
+
+ // Only handle EINVAL. Other errors abort.
+ int new_errno = strerror_return == -1 ? errno : strerror_return;
+ if (new_errno == EINVAL)
+return "";
+
+ _LIBCPP_ASSERT(new_errno == ERANGE, "unexpected error from ::strerror_r");
+ // FIXME maybe? 'strerror_buff_size' is likely to exceed the
+ // maximum error size so ERANGE shouldn't be returned.
+ std::abort();
+}
+
+// This function handles both GNU and POSIX variants, dispatching to
+// one of the two above functions.
string do_strerror_r(int ev) {
char buffer[strerror_buff_size];
+// Preserve errno around the call. (The C++ standard requires that
+// system_error functions not modify errno).
const int old_errno = errno;
-int ret;
-if ((ret = ::strerror_r(ev, buffer, strerror_buff_size)) != 0) {
-// If `ret == -1` then the error is specified using `errno`, otherwise
-// `ret` represents the error.
-const int new_errno = ret == -1 ? errno : ret;
-errno = old_errno;
-if (new_errno == EINVAL) {
-std::snprintf(buffer, strerror_buff_size, "Unknown error %d", ev);
-return string(buffer);
-} else {
-_LIBCPP_ASSERT(new_errno == ERANGE, "unexpected error from
::strerr_r");
-// FIXME maybe? 'strerror_buff_size' is likely to exceed the
-// maximum error size so ERANGE shouldn't be returned.
-std::abort();
-}
+const char *error_message = handle_strerror_r_return(
+::strerror_r(ev, buffer, strerror_buff_size), buffer);
+// If we didn't get any message, print one now.
+if (!error_message[0]) {
+ std::snprintf(buffer, strerror_buff_size, "Unknown error %d", ev);
+ error_message = buffer;
}
-return string(buffer);
+errno = old_errno;
+return string(error_message);
}
#endif
-
} // end namespace
#endif
Index: libcxx/trunk/src/system_error.cpp
===
--- libcxx/trunk/src/system_error.cpp
+++ libcxx/trunk/src/system_error.cpp
@@ -73,39 +73,59 @@
std::snprintf(buffer, strerror_buff_size, "unknown error %d", ev);
return string(buffer);
}
-#elif defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC) && \
-(!defined(__ANDROID__) || __ANDROID_API__ >= 23)
-// GNU Extended version
-string do_strerror_r(int ev) {
-char buffer[strerror_buff_size];
-char* ret = ::strerror_r(ev, buffer, strerror_buff_size);
-return string(ret);
-}
#else
-// POSIX version
+
+// Only one of the two following functions will be used, depending on
+// the return type of strerror_r:
+
+// For the GNU variant, a char* return value:
+__attribute__((unused)) const char *
+handle_strerror_r_return(char *strerror_return, char *buffer) {
+ // GNU always returns a string pointer in its return value. The
+ // string might point to either the input buffer, or a static
+ // buffer, but we don't care which.
+ return strerror_return;
+}
+
+// For the POSIX variant: an int return value.
+__attribute__((unused)) const char *
+handle_strerror_r_return(int strerror_return, char *buffer) {
+ // The POSIX variant either:
+ // - fills in the provided buffer and returns 0
[PATCH] D34294: Rework libcxx strerror_r handling.
jyknight added a comment. EricWF, wdyt? https://reviews.llvm.org/D34294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34294: Rework libcxx strerror_r handling.
krytarowski added a comment. In https://reviews.llvm.org/D34294#782856, @jyknight wrote: > In https://reviews.llvm.org/D34294#782806, @krytarowski wrote: > > > New one is harder to comprehend and less portable (usage of `__atribute__`). > > > I can't disagree more strongly. This is fundamentally portable C++ code -- > __attribute__((unused)) is simply warning suppression. > I used it directly, because I saw other .cpp files in libcxx already did so. > If it needs to be conditioned, it could be. OK with me. https://reviews.llvm.org/D34294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34294: Rework libcxx strerror_r handling.
jyknight added a comment. In https://reviews.llvm.org/D34294#782806, @krytarowski wrote: > New one is harder to comprehend and less portable (usage of `__atribute__`). I can't disagree more strongly. This is fundamentally portable C++ code -- __attribute__((unused)) is simply warning suppression. I used it directly, because I saw other .cpp files in libcxx already did so. If it needs to be conditioned, it could be. > Or better: > #elif __GLIBC__ || (__ANDROID_API__ - 0) >= 23 > If I understand correctly, Android adopted GNU version. No, we can't use that conditional, because android and glibc aren't the only ones to have adopted the GNU variant. As I mentioned, newlib uses it also. We *may* be able to use a different conditional than what's there or what you suggested, but solving the problem once and for all by using the function type is certainly a better solution. https://reviews.llvm.org/D34294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34294: Rework libcxx strerror_r handling.
krytarowski added a comment. Or better: #elif __GLIBC__ || (__ANDROID_API__ - 0) >= 23 If I understand correctly, Android adopted GNU version. https://reviews.llvm.org/D34294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34294: Rework libcxx strerror_r handling.
krytarowski added a comment. New one is harder to comprehend and less portable (usage of `__atribute__`). Can we just replace #elif defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC) && \ (!defined(__ANDROID__) || __ANDROID_API__ >= 23) with `#elif __GLIBC__`? https://reviews.llvm.org/D34294 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34294: Rework libcxx strerror_r handling.
jyknight created this revision.
Herald added subscribers: krytarowski, srhines.
The set of #ifdefs used to handle the two incompatible variants of
strerror_r were not complete (they didn't handle newlib appropriately).
Rather than attempting to make the ifdefs more complex, make them
unnecessary by choosing which behavior to use dependent upon the
return type.
https://reviews.llvm.org/D34294
Files:
src/system_error.cpp
Index: src/system_error.cpp
===
--- src/system_error.cpp
+++ src/system_error.cpp
@@ -73,39 +73,59 @@
std::snprintf(buffer, strerror_buff_size, "unknown error %d", ev);
return string(buffer);
}
-#elif defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC) &&
\
-(!defined(__ANDROID__) || __ANDROID_API__ >= 23)
-// GNU Extended version
-string do_strerror_r(int ev) {
-char buffer[strerror_buff_size];
-char* ret = ::strerror_r(ev, buffer, strerror_buff_size);
-return string(ret);
-}
#else
-// POSIX version
+
+// Only one of the two following functions will be used, depending on
+// the return type of strerror_r:
+
+// For the GNU variant, a char* return value:
+__attribute__((unused)) const char *
+handle_strerror_r_return(char *strerror_return, char *buffer) {
+ // GNU always returns a string pointer in its return value. The
+ // string might point to either the input buffer, or a static
+ // buffer, but we don't care which.
+ return strerror_return;
+}
+
+// For the POSIX variant: an int return value.
+__attribute__((unused)) const char *
+handle_strerror_r_return(int strerror_return, char *buffer) {
+ // The POSIX variant either:
+ // - fills in the provided buffer and returns 0
+ // - returns a positive error value, or
+ // - returns -1 and fills in errno with an error value.
+ if (strerror_return == 0)
+return buffer;
+
+ // Only handle EINVAL. Other errors abort.
+ int new_errno = strerror_return == -1 ? errno : strerror_return;
+ if (new_errno == EINVAL)
+return "";
+
+ _LIBCPP_ASSERT(new_errno == ERANGE, "unexpected error from ::strerror_r");
+ // FIXME maybe? 'strerror_buff_size' is likely to exceed the
+ // maximum error size so ERANGE shouldn't be returned.
+ std::abort();
+}
+
+// This function handles both GNU and POSIX variants, dispatching to
+// one of the two above functions.
string do_strerror_r(int ev) {
char buffer[strerror_buff_size];
+// Preserve errno around the call. (The C++ standard requires that
+// system_error functions not modify errno).
const int old_errno = errno;
-int ret;
-if ((ret = ::strerror_r(ev, buffer, strerror_buff_size)) != 0) {
-// If `ret == -1` then the error is specified using `errno`, otherwise
-// `ret` represents the error.
-const int new_errno = ret == -1 ? errno : ret;
-errno = old_errno;
-if (new_errno == EINVAL) {
-std::snprintf(buffer, strerror_buff_size, "Unknown error %d", ev);
-return string(buffer);
-} else {
-_LIBCPP_ASSERT(new_errno == ERANGE, "unexpected error from
::strerr_r");
-// FIXME maybe? 'strerror_buff_size' is likely to exceed the
-// maximum error size so ERANGE shouldn't be returned.
-std::abort();
-}
+const char *error_message = handle_strerror_r_return(
+::strerror_r(ev, buffer, strerror_buff_size), buffer);
+// If we didn't get any message, print one now.
+if (!error_message[0]) {
+ std::snprintf(buffer, strerror_buff_size, "Unknown error %d", ev);
+ error_message = buffer;
}
-return string(buffer);
+errno = old_errno;
+return string(error_message);
}
#endif
-
} // end namespace
#endif
Index: src/system_error.cpp
===
--- src/system_error.cpp
+++ src/system_error.cpp
@@ -73,39 +73,59 @@
std::snprintf(buffer, strerror_buff_size, "unknown error %d", ev);
return string(buffer);
}
-#elif defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC) && \
-(!defined(__ANDROID__) || __ANDROID_API__ >= 23)
-// GNU Extended version
-string do_strerror_r(int ev) {
-char buffer[strerror_buff_size];
-char* ret = ::strerror_r(ev, buffer, strerror_buff_size);
-return string(ret);
-}
#else
-// POSIX version
+
+// Only one of the two following functions will be used, depending on
+// the return type of strerror_r:
+
+// For the GNU variant, a char* return value:
+__attribute__((unused)) const char *
+handle_strerror_r_return(char *strerror_return, char *buffer) {
+ // GNU always returns a string pointer in its return value. The
+ // string might point to either the input buffer, or a static
+ // buffer, but we don't care which.
+ return strerror_return;
+}
+
+// For the POSIX variant: an int return value.
+__attribute__((unused)) const char *
+handle_strerror_r_return(int strerror_return, char
