-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[libc++] Add support for picolibc and newlib in RUNTIMES_USE_LIBC #147956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -481,6 +481,21 @@ include(config-ix) | |
include(HandleLibC) # Setup the C library flags | ||
include(HandleLibCXXABI) # Setup the ABI library flags | ||
|
||
# Set C library in use to define respective macro in __config_site | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest we do this instead (around line 753 where we handle other
That's more consistent with what we do for the other There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for review! Good point, moved and made use of config_define() for consistency. |
||
# RUNTIMES_USE_LIBC was checked in HandleLibC to be one of accepted values | ||
if (RUNTIMES_USE_LIBC STREQUAL "llvm-libc") | ||
set(LIBCXX_LIBC_LLVMLIBC 1) | ||
elseif (RUNTIMES_USE_LIBC STREQUAL "picolibc") | ||
set(LIBCXX_LIBC_PICOLIBC 1) | ||
# picolibc is derived from newlib and behaves the same in regards to libc++ | ||
# so setting both here: | ||
# * LIBCXX_LIBC_NEWLIB is used now | ||
# * LIBCXX_LIBC_PICOLIBC can be used for further customizations later | ||
set(LIBCXX_LIBC_NEWLIB 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you absolutely need this right now (if so, why)? I think it would be cleaner to have a 1-1 mapping between the macros we define and the libc we're using, otherwise things become confusing when reading the source. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was for completeness only - removed now. |
||
elseif (RUNTIMES_USE_LIBC STREQUAL "newlib") | ||
set(LIBCXX_LIBC_NEWLIB 1) | ||
endif() | ||
|
||
# FIXME(EricWF): See the FIXME on LIBCXX_ENABLE_PEDANTIC. | ||
# Remove the -pedantic flag and -Wno-pedantic and -pedantic-errors | ||
# so they don't get transformed into -Wno and -errors respectively. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,11 @@ | |
// Hardening. | ||
#cmakedefine _LIBCPP_HARDENING_MODE_DEFAULT @_LIBCPP_HARDENING_MODE_DEFAULT@ | ||
|
||
// C libraries | ||
#cmakedefine LIBCXX_LIBC_LLVMLIBC | ||
#cmakedefine LIBCXX_LIBC_PICOLIBC | ||
#cmakedefine LIBCXX_LIBC_NEWLIB | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very good suggestion, done - the checks are much cleaner now. |
||
|
||
// __USE_MINGW_ANSI_STDIO gets redefined on MinGW | ||
#ifdef __clang__ | ||
# pragma clang diagnostic push | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,11 +42,10 @@ | |
# endif | ||
#endif | ||
|
||
// This is required in order for _NEWLIB_VERSION to be defined in places where we use it. | ||
// TODO: We shouldn't be including arbitrarily-named headers from libc++ since this can break valid | ||
// user code. Move code paths that need _NEWLIB_VERSION to another customization mechanism. | ||
// TODO: Remove this deprecated behavior after LLVM 22 release | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is something that vendors should change, I'm not sure there's a benefit from deprecating this before we remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, I do not expect many people using it anyway - removed. |
||
// To build libc++ with picolibc provide RUNTIMES_USE_LIBC=picolibc | ||
#if __has_include(<picolibc.h>) | ||
# include <picolibc.h> | ||
# define LIBCXX_LIBC_NEWLIB | ||
pratlucas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#endif | ||
|
||
#ifndef __BYTE_ORDER__ | ||
|
voltur01 marked this conversation as resolved.
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment on the whole patch: Let's use
_LIBCPP_foo
instead ofLIBCXX_foo
for the macro names. That's what we do consistently everywhere, and we must use a reserved identifier.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, done.