[libcxx-commits] [PATCH] D154246: [libc++] Add check for building with picolibc

[Alexander Richardson via Phabricator via libcxx-commits]

arichardson added a comment.

One suggestion otherwise looking good to me



================
Comment at: libcxx/test/configs/armv7m-libc++.cfg.in:17
+    # which for picolibc is defined in picolibc.h.
+    ' -include picolibc.h'
+))
----------------
domin144 wrote:
> arichardson wrote:
> > michaelplatings wrote:
> > > arichardson wrote:
> > > > Doesn't this mean the library can't be used without adding this include to any build system that is trying to use libc++ with picolibc? I think it would be a lot cleaner if we could just add the required includes to the libc++ source code?
> > > > 
> > > > As far as I recall from the last time I tried to use libc++ with picolibc this was not needed but I may have been building a slightly different configuration
> > > > Doesn't this mean the library can't be used without adding this include to any build system that is trying to use libc++ with picolibc?
> > > 
> > > Most libc++ headers already indirectly include picolibc.h so most of the time this isn't a problem in practice. It's a minority of tests that fail without this include.
> > > 
> > > > I think it would be a lot cleaner if we could just add the required includes to the libc++ source code?
> > > 
> > > I agree with you that it would be cleaner to remove this line and instead change the libc++ headers. However I can imagine that adding extra includes could cause undesirable knock-on effects. I think it's best to limit the scope of this change to only adding the test setup and not touching libc++ itself.
> > Do you have a list of tests that fail? If it's just a few maybe the better solution is to add the fixme to REQUIRES?
> There are 138 tests that fail without the ' -include picolibc.h'.
> I haven't reviewed them all, but most of them fall into two categories:
> - use of `__input` reserved name in stdlib.h (e.g. algorithm.compile.pass.cpp)
> - `ld.lld: error: undefined symbol: std::__1::basic_streambuf<char, std::__1::char_traits<char>>::seekoff(long long, std::__1::ios_base::seekdir, unsigned int)` (e.g. eof.pass.cpp)
> I would prefer to solve it in a separate patch as this might need modification to libc++ headers.
If it's so many tests then yes that sounds good to me.


================
Comment at: libcxx/test/libcxx/system_reserved_names.gen.py:105
+// picolibc uses __input as a parameter name of a64l & l64a
+#ifndef _NEWLIB_VERSION
+# define __input SYSTEM_RESERVED_NAME
----------------
michaelplatings wrote:
> ldionne wrote:
> > What is the relationship between picolibc and `_NEWLIB_VERSION`?
> Picolibc is derived from Newlib, so defines _NEWLIB_VERSION. I've updated the comment to make it clear that it applies to both libraries.
So it seems this header needs the picolibc.h include? Could we just add something like #include <stdlib.h> to the top of this file?


================
Comment at: libcxxabi/test/configs/armv7m-libc++abi.cfg.in:29
+# using this feature.
+config.available_features.add('LIBCXX-PICOLIBC-FIXME')
+
----------------
I believe this is not needed anymore since it's in features.oy now?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154246/new/

https://reviews.llvm.org/D154246