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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

This looks pretty clean, a lot cleaner than I would have expected for an embedded c library. I have a few questions and comments but I don't see major blockers at this point.



================
Comment at: libcxx/include/__atomic/cxx_atomic_impl.h:318-323
+_LIBCPP_DIAGNOSTIC_PUSH
+#  ifdef __arm__
+// Disable warning "large atomic operation may incur significant performance
+// penalty; the access size (4 bytes) exceeds the max lock-free size (0  bytes)"
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Watomic-alignment")
+#  endif
----------------
I am not certain we want to disable this warning here. At the end of the day, it seems like the compiler is warning us about something legitimate, no? We could perhaps silence that warning in the `armv7m` testing configuration instead?


================
Comment at: libcxx/test/configs/armv7m-libc++.cfg.in:1-2
+# This testing configuration handles running the test suite against LLVM's libc++
+# using a static library.
+
----------------
This comment seems copy-pasted (same for the libc++abi one and probably the libunwind one too).


================
Comment at: libcxx/test/configs/armv7m-libc++.cfg.in:12
+    '-nostdinc++ -I %{include} -I %{target-include} -I %{libcxx}/test/support'
+    ' -include picolibc.h'
+))
----------------
Why is that include needed?


================
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
----------------
What is the relationship between picolibc and `_NEWLIB_VERSION`?


================
Comment at: libcxx/utils/ci/run-buildbot:639
+        --install-dir "${INSTALL_DIR}" \
+        --target armv7m-none-eabi
+
----------------
It's weird that you're passing this here but also hardcoding it in your `Armv7M-picolibc.cmake` cache file. IMO it would make more sense *not* to hardcode it in the cache.


================
Comment at: libcxx/utils/libcxx/test/features.py:104
+            std::atomic_uint64_t x;
+            int main(int, char**) { (void)x.load(); }
+          """,
----------------
Otherwise this test will fail in `-ffreestanding` mode, where `main()` isn't special.


================
Comment at: libcxx/utils/libcxx/test/features.py:207
+              int fd[2];
+              return pipe(fd);
             }
----------------
michaelplatings wrote:
> michaelplatings wrote:
> > arichardson wrote:
> > > Why pipe? Is that function actually called inside libc++?
> > `check_assertion.h` uses pipe, fork, exec etc. Picolibc declares these in `unistd.h` but doesn't implement them. Therefore this feature check was previously passing with picolibc, but now fails at link time.
> (Despite the name "has-unix-headers", the check is mostly (always?) used to xfail tests that use `check_assertion.h`)
Yeah it's pretty lame, we need to change that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154246