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

[Michael Platings via Phabricator via libcxx-commits]

michaelplatings marked 3 inline comments as done.
michaelplatings added inline comments.


================
Comment at: libcxx/test/std/atomics/atomics.types.generic/integral.pass.cpp:9
 
+// "undefined symbol: __atomic_load_8"
+// XFAIL: LIBCXX-PICOLIBC-FIXME
----------------
philnik wrote:
> I guess this is caused by a missing libatomic?
Yes, there's no libatomic or equivalent. Perhaps something like `REQUIRES: atomic` would be better here.


================
Comment at: libcxx/utils/ci/run-buildbot:608
+    clean
+    ${MONOREPO_ROOT}/libcxx/utils/ci/build-picolibc.sh \
+        --build-dir "${BUILD_DIR}" \
----------------
philnik wrote:
> DavidSpickett wrote:
> > michaelplatings wrote:
> > > DavidSpickett wrote:
> > > > DavidSpickett wrote:
> > > > > Mordante wrote:
> > > > > > Why do we need to build picolib every CI run instead of storing it in the base image?
> > > > > > If that is really needed we should add some documentation why it's needed.
> > > > > Assuming we can find a way to use this script directly in Linaro's container builds, it's not a problem. That ensures anyone can reproduce it easily.
> > > > > 
> > > > > The justification for picking a fixed version should be documented somewhere even if it's just that it's whatever the current version was at the time.
> > > > The other problem doing this is how do we place the library in a place that would also be appropriate for anyone reproducing this without the container itself. I guess people could set an env var for the linker, but it's one more difference they have to account for.
> > > > 
> > > > Building it each time keeps it isolated, at the cost of compile time each time.
> > > Yes I'd like to get this working in a relatively environment-independent state at first. Once that's stabilised we can put picolibc into the relevant container(s) and that will speed things up by ~16 seconds. The only additional dependencies needed are python3-venv & python3-pip (for meson) which I //think// are already included in the Linaro containers.
> > Correct, venv and pip3 are included already for running the llvm test suite (the image is used for many different bots).
> Personally I'm not super concerned about building picolibc every time. I don't expect it to be a long build (probably similar to libc++), and testing always the latest version seems like a nice thing to me.
Yes, the time to build picolibc is seconds - negligible compared to the testing time. The revision of picolibc is pinned, but doing it this way does make it trivial to update to the latest version.


================
Comment at: libcxx/utils/libcxx/test/features.py:207
+              int fd[2];
+              return pipe(fd);
             }
----------------
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.


================
Comment at: libunwind/test/configs/armv7m-libunwind.cfg.in:20
+))
+config.substitutions.append(('%{exec}',
+    'true'
----------------
arichardson wrote:
> There should probably be a comment here such as `todo: run using qemu-system-arm`?
Agreed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154246