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

[Dominik Wójt via Phabricator via libcxx-commits]

domin144 added inline comments.


================
Comment at: libcxx/test/configs/armv7m-libc++.cfg.in:17
+    # which for picolibc is defined in picolibc.h.
+    ' -include picolibc.h'
+))
----------------
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.


================
Comment at: libcxx/test/configs/armv7m-libc++.cfg.in:38
+# using this feature.
+config.available_features.add('LIBCXX-PICOLIBC-FIXME')
+
----------------
arichardson wrote:
> michaelplatings wrote:
> > arichardson wrote:
> > > Could this be autodetected instead by checking for the presence of the picolibc header? Then it would also work out of the box with the default config file.
> > I don't think we can infer that we're targeting picolibc from the presence of the header. It might also happen to be on an include path in a scenario in which another platform is targeted. I think it's best to be explicit here.
> I meant trying to compile something like the following:
> ```
> #include <stdio.h>
> #ifndef __PICOLIBC__
> #error not picolibc
> #endif
> ```
Thanks! Applied in the new diff.


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

https://reviews.llvm.org/D154246