This is an archive of the discontinued LLVM Phabricator instance.

Add initial support for the MUSL C library.
ClosedPublic

Authored by vkalintiris on Oct 12 2015, 2:10 PM.

Details

Summary

This patch adds the LIBCXX_LIBC_IS_MUSL cmake option to allow the
building of libcxx with the Musl C library. The option is necessary as
Musl does not provide any predefined macro in order to test for its
presence, like GLIBC. Most of the changes specify the correct path to
choose through the various #if/#else constructs in the locale code.

Depends on D13407.

Diff Detail

Repository
rL LLVM

Event Timeline

vkalintiris retitled this revision from to Add initial support for the MUSL C library..
vkalintiris updated this object.
vkalintiris added a subscriber: cfe-commits.
jroelofs edited edge metadata.Oct 12 2015, 2:18 PM

LGTM, with one small nit (and once D13407 lands):

src/locale.cpp
1176 ↗(On Diff #37169)
s/|| ||/||/
jroelofs accepted this revision.Oct 13 2015, 3:22 PM
jroelofs edited edge metadata.

D13407 has landed.

This revision is now accepted and ready to land.Oct 13 2015, 3:22 PM
EricWF requested changes to this revision.Oct 13 2015, 3:30 PM
EricWF edited edge metadata.

I don't think this is ready to go. Please don't commit yet. Sorry @jroelofs.

CMakeLists.txt
309 ↗(On Diff #37169)

Please name the macro "_LIBCPP_HAS_MUSL_LIBC" for consistency.

include/__config
370 ↗(On Diff #37169)

Where does the __GLIBC__ macro definition come from? I thought it came from the C library headers but we don't include any!

This revision now requires changes to proceed.Oct 13 2015, 3:30 PM
jroelofs added inline comments.Oct 13 2015, 3:48 PM
include/__config
370 ↗(On Diff #37169)

No need for the check to look at GLIBC anyway... _LIBCXX_LIBC_IS_MUSL would be available here, so you should be testing for: !defined(_LIBCXX_LIBC_IS_MUSL) instead (or however that flag ends up being named).

include/support/musl/xlocale.h
10 ↗(On Diff #37169)

This comment was cargo-culted from include/support/xlocale/xlocale.h... Given that this is musl-specific, I don't think it makes sense to call this file "shared" in the same sense that the other one is.

EricWF added inline comments.Oct 13 2015, 4:07 PM
include/__config
370 ↗(On Diff #37169)

Sounds good to me.

vkalintiris edited edge metadata.

Address reviewers' comments.

vkalintiris added inline comments.Oct 14 2015, 4:29 AM
CMakeLists.txt
309 ↗(On Diff #37169)

Should I rename the corresponding cmake option too?

EricWF requested changes to this revision.Oct 14 2015, 12:37 PM
EricWF edited edge metadata.

Thanks for the quick turn around. This patch is almost ready to land but still needs a couple more changes.

include/__config
387 ↗(On Diff #37335)
#else // defined(_LIBCPP_HAS_MUSL_LIBC)
include/__config_site.in
21 ↗(On Diff #37335)

No need for whitespace.

include/__locale
40 ↗(On Diff #37335)

Since musl depends on support headers you should add a CMake check that ensures LIBCXX_INSTALL_SUPPORT_HEADERS is not turned off when LIBCXX_LIBC_IS_MUSL is ON.

include/support/musl/xlocale.h
21 ↗(On Diff #37335)

It looks like this header uses more symbols than are just provided by cstdlib. Please add the required includes.

src/locale.cpp
958 ↗(On Diff #37335)

Where did the __NetBSD__ case go?

This revision now requires changes to proceed.Oct 14 2015, 12:37 PM

Thanks for the quick turn around. This patch is almost ready to land but still needs a couple more changes.

CMakeLists.txt
320 ↗(On Diff #37335)

Yes please, I wasn't going to ask but I would prefer LIBCXX_HAS_MUSL_LIBC.

vkalintiris edited edge metadata.
vkalintiris marked 6 inline comments as done.

Sorry for the delay.

Addressed reviewer comments and modifed CMake to check that
LIBCXX_INSTALL_SUPPORT_HEADERS is not turned off when LIBCXX_LIBC_IS_MUSL is ON.

vkalintiris added inline comments.Oct 20 2015, 7:16 AM
src/locale.cpp
958 ↗(On Diff #37335)

It's in the previous #line directive, check line 956.

Thanks for the update, I think this should be good to go. I'll give it a final once over tonight.

src/locale.cpp
958 ↗(On Diff #37850)

Ah, sorry I missed that. So this was an existing bug. Thanks.

Thanks for the update, I think this should be good to go. I'll give it a final once over tonight.

Ping.

vkalintiris edited edge metadata.

s/__NetBSD_\)/__NetBSD__\)/

EricWF accepted this revision.Nov 6 2015, 1:38 PM
EricWF edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Nov 6 2015, 1:38 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review!

Everything seems fine from the buildbots except for a single test that failed:

http://lab.llvm.org:8011/builders/libcxx-libcxxabi-x86_64-linux-debian/builds/730

The next build (731) of that buildbot was green. Is libc++::try_lock_shared_for.pass.cpp a known flakey test?

Yep. That's a flaky test. No need to worry.

@vkalintiris @jroelofs Is it possible to detect Musl through some macro? I'd like to get rid of the CMake option -- this isn't the sort of property that we want to set explicitly at configure time, it's the sort of property we want to sniff out in the library instead.

@vkalintiris @jroelofs Is it possible to detect Musl through some macro? I'd like to get rid of the CMake option -- this isn't the sort of property that we want to set explicitly at configure time, it's the sort of property we want to sniff out in the library instead.

I wish there was. Last I remember talking about it, Rich Felker was really against having one. I doubt this has changed.

Thanks for the context. This is a very puristic point of view from the musl author.. not very useful. There are things that are difficult to detect with a check like __has_feature or similar, and having a macro to detect the library is necessary.