This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ldionne on Feb 26 2021, 8:24 AM.

Details

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.

Diff Detail

Event Timeline

jasonliu requested review of this revision.Feb 26 2021, 8:24 AM
jasonliu created this revision.

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

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.

Gentle ping.

Quuxplusone added a subscriber: 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.

This revision is now accepted and ready to land.Mar 12 2021, 6:39 AM
jasonliu updated this revision to Diff 330739.Mar 15 2021, 11:32 AM

Address Arthur's comment.

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

jasonliu marked 2 inline comments as done.Mar 15 2021, 11:35 AM
jasonliu updated this revision to Diff 331347.Mar 17 2021, 12:39 PM
jasonliu edited the summary of this revision. (Show Details)

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

@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 updated this revision to Diff 331351.Mar 17 2021, 1:05 PM

Rebase so that buildbot could build.

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?

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 accepted this revision.Mar 18 2021, 10:51 AM

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.

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.

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 updated this revision to Diff 332740.Mar 23 2021, 11:25 AM

Use "C" locale when NULL is passed.

jasonliu marked an inline comment as done.Mar 23 2021, 11:28 AM
jasonliu added inline comments.
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.

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 updated this revision to Diff 332775.Mar 23 2021, 1:34 PM

Use thread_local for newlocale variable.

libcxx/include/__support/ibm/xlocale.h
235–238

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

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 updated this revision to Diff 332802.Mar 23 2021, 3:17 PM

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

LGTM with small suggested changes.

libcxx/include/__support/ibm/xlocale.h
233–235

Use double underscore for local variable name too.

251

Same comment.

258
265
272
279
286
293
SeanP added a subscriber: SeanP.Mar 24 2021, 1:11 PM
SeanP added inline comments.
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 added a subscriber: muiez.Mar 24 2021, 1:41 PM
muiez added inline comments.
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 updated this revision to Diff 333110.Mar 24 2021, 1:44 PM
jasonliu marked 2 inline comments as done.

Use freelocale instead to avoid using thread local on Z.

jasonliu marked 6 inline comments as done.Mar 24 2021, 1:45 PM

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

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

LGTM; thanks!

This revision was landed with ongoing or failed builds.Mar 24 2021, 3:15 PM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 24 2021, 3:15 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cdar3939 reopened this revision.Jan 3 2022, 10:02 AM
ldionne commandeered this revision.Jan 3 2022, 10:41 AM
ldionne edited reviewers, added: jasonliu; removed: ldionne.
ldionne added a subscriber: cdar3939.

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.

ldionne accepted this revision.Jan 3 2022, 10:43 AM
This revision is now accepted and ready to land.Jan 3 2022, 10:43 AM
ldionne closed this revision.Jan 3 2022, 10:43 AM