[libc++][AIX] Initial patch to unblock the libc++ build on AIX

[ldionne]

Summary

While we are waiting for the AIX buildbot to be setup correctly: http://lab.llvm.org:8014/#/builders/144.
Here's the patch that would unblock the build of libc++ library on AIX:

1. Add _AIX guard for _LIBCPP_HAS_THREAD_API_PTHREAD
2. Add missing strtod_l and strtof_l which are needed for the build, and use uselocale to actually take the locale setting into account.
3. extract_mtime and extract_atime mod needed for AIX. As stat structure on AIX uses internal structure st_timespec to store time for binary compatibility reason. So we need to convert it back to timespec here.
4. Do not build cxa_thread_atexit.cpp for libcxxabi on AIX.

[curdeius]

It would be cool if this builder got added to the precommit CI. Any plans on that?

[jasonliu]

In D97558#2590597, @curdeius wrote:

It would be cool if this builder got added to the precommit CI. Any plans on that?

Yes, after we get a stable build of libcxx/libcxxabi/libunwind, we will try to add it to the precommit CI.

[jasonliu]

Gentle ping.

[• Quuxplusone]

Looks harmless to me: I know nothing about AIX, but I don't see any way for this patch to affect other targets. :) Please wait for "libc++" approval from a second approver.

libcxx/include/__support/ibm/xlocale.h

231

explicit :)

241

libc++ has no consistent style for end-of-namespace comments, but we should put something on this line.

} // namespace
} // end namespace
} // anonymous namespace

I see instances of 1 and 2 in libc++; I do 3 in my own codebases. I think 1 is probably what I'd do here, for consistency.

[jasonliu]

Address Arthur's comment.

[jasonliu]

Thanks @Quuxplusone for the review.
Ping for a second reviewer in libc++ review group.

[jasonliu]

Added do not build cxa_thread_atexit.cpp for libcxxabi on AIX.

[jasonliu]

@Quuxplusone While waiting for the second reviewer, I added an extra change since you approved: do not build cxa_thread_atexit.cpp on AIX.
Could you take a quick look to see if that's okay?

Ping for a second reviewer.
Thanks!

[jasonliu]

Rebase so that buildbot could build.

[• Quuxplusone]

Still looks hygienic enough to me, from a libc++ point of view.
Are all those people whose names I don't recognize AIX people? Any of them want to chime in with a "yes, this looks appropriate for our target"?

libcxx/include/__support/ibm/xlocale.h

214

Btw, why _AIX elsewhere but _AIX72 here?

[jasonliu]

Are all those people whose names I don't recognize AIX people? Any of them want to chime in with a "yes, this looks appropriate for our target"?

@xingxue @daltenty @hubert.reinterpretcast @Xiangling_L also works on AIX. So I will wait for at least one of them to say yes as well.

libcxx/include/__support/ibm/xlocale.h

214

We don't need this definition for AIX7.2 and up because strftime_l is available since AIX7.2. And when we are on AIX7.3, we would still define macros like _AIX71 and _AIX72. So _AIX72 guard here would only let the below definition exposed on 7.1 and below.

[ldionne]

This LGTM but I would really really like to have a pre-commit node set up for this platform if we are going to support it properly.

[jasonliu]

In D97558#2635187, @ldionne wrote:

This LGTM but I would really really like to have a pre-commit node set up for this platform if we are going to support it properly.

@ldionne Thanks for the review. pre-commit CI is a goal that we are working towards to. I assume that we would need to at least be able to build libcxx/libcxxabi/libunwind on AIX in order to setup a useful pre-commit CI.
So my current plan is to enable the building for those components first. Make sure the buildbot works. Move to pre-commit CI (but not running the tests). Then upstream patches to clear the lit tests. Then enable lit test for pre-commit CI.
Let me know if there is any other suggestions for setting up the pre-commit CI.

[jasonliu]

Ping for reviews from AIX's perspective. @xingxue @daltenty @hubert.reinterpretcast @Xiangling_L

[hubert.reinterpretcast]

libcxx/include/__support/ibm/xlocale.h

232

Do we want to do the NULL -> "C" replacement documented in places like: https://www.freebsd.org/cgi/man.cgi?query=xlocale&sektion=3&format=html
?

libcxx/src/filesystem/filesystem_common.h

472

Noting that the code needed for compiling with _XOPEN_SOURCE >= 700 is probably different.

[jasonliu]

Use "C" locale when NULL is passed.

[jasonliu]

libcxx/src/filesystem/filesystem_common.h

472

I think by default we are having ALL_SOURCE which defines _XOPEN_SOURCE as 700. But I think compiling with different macros with libc++ headers on AIX is something we need to consider later. For example, __LARGE_FILES might affect how it compiles in this case too.

[hubert.reinterpretcast]

libcxx/include/__support/ibm/xlocale.h

233

I think a block-scope thread_local variable may work better of having a "C" locale_t. The code as-is looks to be resource-leaking.

libcxx/src/filesystem/filesystem_common.h

472

Thanks for looking into this. I missed that st_mtime, etc. were defined as macros in cases where they don't exist as a member of the structure.

[jasonliu]

Use thread_local for newlocale variable.

[hubert.reinterpretcast]

libcxx/include/__support/ibm/xlocale.h

235–238

The call to uselocale seems to have gone missing in the mix.

[hubert.reinterpretcast]

libcxx/include/__support/ibm/xlocale.h

230

In case of user-defined macros, we need to add double-underscore to the names we're using for the class, the class members, the function parameters, etc.

[jasonliu]

Add back setlocale and use __ prefix in case of user macro expansion.

[hubert.reinterpretcast]

LGTM with small suggested changes.

libcxx/include/__support/ibm/xlocale.h

233–235

Use double underscore for local variable name too.

256

Same comment.

263

270

277

284

291

298

[SeanP]

libcxx/include/__support/ibm/xlocale.h

233

thread_local isn't supported on z/OS. Can you rework this part so it compiles on z/OS too.

[muiez]

libcxx/include/__support/ibm/xlocale.h

233

Maybe we can make use of the HAS_THREAD_LOCAL macro to safeguard thread_local? I've seen the macro being used in libcxxabi/src/cxa_exception_storage.cpp

[jasonliu]

Use freelocale instead to avoid using thread local on Z.

[jasonliu]

@hubert.reinterpretcast Does the freelocale approach look good to you?

[hubert.reinterpretcast]

In D97558#2648876, @jasonliu wrote:

@hubert.reinterpretcast Does the freelocale approach look good to you?

LGTM; thanks!

[ldionne]

Commandeering to close again. @cdar3939 seems like a spam account, created today and re-opening this randomly. I disabled their Phab account.

@cdar3939 if I am getting this wrong, please mention it and I can re-enable your account.

libcxx/include/__config

#  if defined(__FreeBSD__) || \
       defined(__OpenBSD__) || \
       defined(__NuttX__) || \
       defined(__linux__) || \
       defined(__GNU__) || \
       defined(__APPLE__) || \
       defined(__CloudABI__) || \
       defined(__sun__) || \
       defined(__MVS__) || \
+      defined(_AIX) || \
       (defined(__MINGW32__) && __has_include(<pthread.h>))
 #    define _LIBCPP_HAS_THREAD_API_PTHREAD
 #  elif defined(__Fuchsia__)
      // TODO(44575): Switch to C11 thread API when possible.
 #    define _LIBCPP_HAS_THREAD_API_PTHREAD
 #  elif defined(_LIBCPP_WIN32API)
 #    define _LIBCPP_HAS_THREAD_API_WIN32
 #  else

libcxx/include/__support/ibm/xlocale.h

size_t wcsxfrm_l(wchar_t *__ws1, const wchar_t *__ws2, size_t __n,
     locale_t locale)
 {
   return __xwcsxfrm(locale, __ws1, __ws2, __n);
 }
 #endif // !defined(_AIX71)
 
 // strftime_l() is defined by POSIX. However, AIX 7.1 and z/OS do not have it
 // implemented yet. z/OS retrieves it from the POSIX fallbacks.
+#if !defined(_AIX72)
 static inline
 size_t strftime_l(char *__s, size_t __size, const char *__fmt,
                   const struct tm *__tm, locale_t locale) {
   return __xstrftime(locale, __s, __size, __fmt, __tm);
 }
+#endif
 
 #elif defined(__MVS__)
 #include <wctype.h>
 // POSIX routines
 #include <__support/xlocale/__posix_l_fallback.h>
 #endif // defined(__MVS__)
 
+namespace {
+
+struct SetAndRestore {
+  explicit SetAndRestore(locale_t locale) {
+    if (locale == (locale_t)0) {
+      thread_local locale_t cloc =
+          newlocale(LC_ALL_MASK, "C", /* base */ (locale_t)0);
+      stored = cloc;
+    } else {
+      stored = locale;
+    }
+  }
+
+  ~SetAndRestore() {
+    uselocale(stored);
+  }
+
+private:
+  locale_t stored;
+};
+
+} // namespace
+
 // The following are not POSIX routines.  These are quick-and-dirty hacks
 // to make things pretend to work
 static inline
 long long strtoll_l(const char *__nptr, char **__endptr,
     int __base, locale_t locale) {
+  SetAndRestore newloc(locale);
   return strtoll(__nptr, __endptr, __base);
 }
 
 static inline
 long strtol_l(const char *__nptr, char **__endptr,
     int __base, locale_t locale) {
+  SetAndRestore newloc(locale);
   return strtol(__nptr, __endptr, __base);
 }
 
 static inline
-double strtod_l(const char *__nptr, char **__endptr,
-    locale_t locale) {
-  return strtod(__nptr, __endptr);
-}
-
-static inline
-float strtof_l(const char *__nptr, char **__endptr,
-    locale_t locale) {
-  return strtof(__nptr, __endptr);
-}
-
-static inline
 long double strtold_l(const char *__nptr, char **__endptr,
     locale_t locale) {
+  SetAndRestore newloc(locale);
   return strtold(__nptr, __endptr);
 }
 
 static inline
 unsigned long long strtoull_l(const char *__nptr, char **__endptr,
     int __base, locale_t locale) {
+  SetAndRestore newloc(locale);
   return strtoull(__nptr, __endptr, __base);
 }
 
 static inline
 unsigned long strtoul_l(const char *__nptr, char **__endptr,
     int __base, locale_t locale) {
+  SetAndRestore newloc(locale);
   return strtoul(__nptr, __endptr, __base);
 }
 
 static inline
+double strtod_l(const char *__nptr, char **__endptr,
+    locale_t locale) {
+  SetAndRestore newloc(locale);
+  return strtod(__nptr, __endptr);
+}
+
+static inline
+float strtof_l(const char *__nptr, char **__endptr,
+    locale_t locale) {
+  SetAndRestore newloc(locale);
+  return strtof(__nptr, __endptr);
+}
+
+static inline
 int vasprintf(char **strp, const char *fmt, va_list ap)
 {
   const size_t buff_size = 256;
   int str_size;
   if ((*strp = (char *)malloc(buff_size)) == NULL)
   {
     return -1;
   }

libcxx/src/filesystem/filesystem_common.h

 inline TimeSpec extract_mtime(StatT const& st) {
   TimeSpec TS = {st.st_mtime, 0};
   return TS;
 }
 inline TimeSpec extract_atime(StatT const& st) {
   TimeSpec TS = {st.st_atime, 0};
   return TS;
 }
+#elif defined(_AIX)
+inline TimeSpec extract_mtime(StatT const& st) {
+  TimeSpec TS = {st.st_mtime, st.st_mtime_n};
+  return TS;
+}
+inline TimeSpec extract_atime(StatT const& st) {
+  TimeSpec TS = {st.st_atime, st.st_atime_n};
+  return TS;
+}
 #else
 inline TimeSpec extract_mtime(StatT const& st) { return st.st_mtim; }
 inline TimeSpec extract_atime(StatT const& st) { return st.st_atim; }
 #endif
 
 #if !defined(_LIBCPP_WIN32API)
 inline TimeVal make_timeval(TimeSpec const& ts) {
   using namespace chrono;

libcxxabi/src/CMakeLists.txt

list(APPEND LIBCXXABI_SOURCES
     cxa_personality.cpp
   )
 else()
   list(APPEND LIBCXXABI_SOURCES
     cxa_noexception.cpp
   )
 endif()
 
-if (LIBCXXABI_ENABLE_THREADS AND (UNIX OR FUCHSIA) AND NOT (APPLE OR CYGWIN))
+if (LIBCXXABI_ENABLE_THREADS AND (UNIX OR FUCHSIA) AND NOT (APPLE OR CYGWIN)
+    AND NOT (${CMAKE_SYSTEM_NAME} MATCHES "AIX"))
   list(APPEND LIBCXXABI_SOURCES
     cxa_thread_atexit.cpp
   )
 endif()
 
 set(LIBCXXABI_HEADERS
   ../include/cxxabi.h
 )