Page MenuHomePhabricator

[libcxx] Allow checking for newlib without using _NEWLIB_VERSION.
Needs RevisionPublic

Authored by abidh on Oct 8 2020, 6:10 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Summary

Existing code in libc++ uses _NEWLIB_VERSION to check for the presence
of newlib. But this macro is only set when a library header has been
included before that brings in its definition. In some cases, this macro
can't be used as a library header has not been included before it use.

This patch adds a CMake variable and corresponding macro that is used
to check for the presence of newlib in the following 2 cases:

  1. The __config header is the generally the first header included. So

_NEWLIB_VERSION used in it will not be set. We check for
_LIBCPP_HAS_NEWLIB_LIBC and include newlib.h that will make sure that
_NEWLIB_VERSION is correctly defined.

  1. The nasty_macros.h also has similar issue so _NEWLIB_VERSION can't be

used in it. We check for _LIBCPP_HAS_NEWLIB_LIBC to stub out a
problematic macro for newlib.

The new variable for newlib is quite similar to existing variable for MUSL.

Diff Detail

Unit TestsFailed

TimeTest
930,581 msApple system (https://buildkite.com/llvm-project/libcxx-ci/builds/74#b232b27c-545c-4d02-9167-97eb4610febf)
--- Generating CMake loading initial cache file /private/tmp/buildkite-agent/builds/libcxx-mbp-local-1/llvm-project/libcxx-ci/libcxx/cmake/caches/Apple.cmake -- The C compiler identification is AppleClang 12.0.0.12000032
896,682 msApple system -fno-exceptions (https://buildkite.com/llvm-project/libcxx-ci/builds/74#e0d623df-0031-410a-900c-b5e30c28d546)
--- Generating CMake loading initial cache file /private/tmp/buildkite-agent/builds/libcxx-mbp-local-1/llvm-project/libcxx-ci/libcxx/cmake/caches/Apple.cmake -- The C compiler identification is AppleClang 12.0.0.12000032
2,592,488 msMacOS C++20 (https://buildkite.com/llvm-project/libcxx-ci/builds/74#5d6d5ad0-27cc-4de5-b6d7-1b7b04d49464)
--- Generating CMake loading initial cache file /usr/local/var/buildkite-agent/builds/2019-local-1/llvm-project/libcxx-ci/libcxx/cmake/caches/Generic-cxx2a.cmake -- The C compiler identification is AppleClang 12.0.0.12000032
380,390 ms-fno-exceptions (https://buildkite.com/llvm-project/libcxx-ci/builds/74#721d9a5e-0eb6-4c95-9b48-763c73d9182f)
--- Generating CMake loading initial cache file /home/libcxx-builder/.buildkite-agent/builds/f30586ed6993-1/llvm-project/libcxx-ci/libcxx/cmake/caches/Generic-noexceptions.cmake -- The C compiler identification is Clang 11.0.0
717,593 msASAN (https://buildkite.com/llvm-project/libcxx-ci/builds/74#16ee2a5a-753b-4c2e-9bd6-25b159fa2d64)
--- Generating CMake loading initial cache file /home/libcxx-builder/.buildkite-agent/builds/e63f4ae75f29-1/llvm-project/libcxx-ci/libcxx/cmake/caches/Generic-asan.cmake -- The C compiler identification is Clang 11.0.0
View Full Test Results (3 Failed · 13 Passed)

Event Timeline

abidh created this revision.Oct 8 2020, 6:10 AM
abidh requested review of this revision.Oct 8 2020, 6:10 AM
ldionne requested changes to this revision.Oct 21 2020, 5:28 AM

I see a couple issues with this:

  1. I don't want to add a new CMake option just to detect an underlying libc. We should be able to detect it through other means. Actually why do we even need _LIBCPP_HAS_NEWLIB_LIBC?
  2. We should have testers for this configuration. Newlib is not currently supported (well.. not officially at least), and we need some kind of testing if we pretend we're supporting it.
This revision now requires changes to proceed.Oct 21 2020, 5:28 AM
abidh added a comment.Oct 22 2020, 3:07 AM

Thanks for the review.

I see a couple issues with this:

Actually why do we even need _LIBCPP_HAS_NEWLIB_LIBC?

Consider example of __config, it already has some code conditional on _NEWLIB_VERSION. But this macro will always be undefined in this file as nothing is bringing in its definition. Note that _NEWLIB_VERSION is not defined by the compiler but it needs newlib.h to be included either directly or indirectly through some other library header. This is what _LIBCPP_HAS_NEWLIB_LIBC was trying to do. Detect newlib where we could not use _NEWLIB_VERSION.

  1. I don't want to add a new CMake option just to detect an underlying libc. We should be able to detect it through other means.

One alternate I could think of what to use -include newlib.h in my compiler flags. That works but the problem with nasty_macros.h still remains unless I use an override of configure_compile_flags_header_includes in my customized config file to skip the check for nasty_macros.h. This can work but it will require all newlib users to handle these things in their own config files.

  1. We should have testers for this configuration. Newlib is not currently supported (well.. not officially at least), and we need some kind of testing if we pretend we're supporting it.

You mentioned in https://reviews.llvm.org/D88825 that you will write some instruction. I will be happy to setup one.