[libcxx-commits] [PATCH] D118503: [SystemZ][z/OS] Add ASCII and 32-bit variants for libc++.

Wed Mar 2 09:10:52 PST 2022

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

I can't support this patch as-is. It upstreams vendor-specific quirks for a single platform, causing the generic upstream code to be side-by-side with platform-specific code for z/OS only. Please try to put yourself in the shoes of the open-source community that doesn't work on z/OS on a day to day basis: this patch basically adds a bunch of platform-specific complexity (i.e. tech debt) that everyone will have to deal with, for essentially only z/OS's benefit. This is not acceptable.

Take for instance the example of Fuchsia, another platform that uses libc++. @phosek added support for a bunch of stuff to support Fuchsia's build, for example `LIBCXX_HERMETIC_STATIC_LIBRARY` & friends, or `LIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY`. Even though I'm not a fan of those because they increase complexity, they are generic and one could argue they are generally useful, so it makes sense to try and support them upstream. I am not sure whether Fuchsia is using the CMake build at this point (I don't think it does yet), however my point is this: how many references to Fuchsia specific stuff can you count in our `CMakeLists.txt`? Very few, because they found ways to implement most of what they needed generically.

Similarly, at Apple, we used to build libc++ and libc++abi with a Xcode project a couple of years ago. I did a bunch of work to migrate that build over to CMake, and in doing so, I had to introduce a bunch of Apple-specific tweaks to the CMake build. I have been upstreaming things that make sense one tiny bit at a time, but even three years later, we are still living with a downstream diff that I don't think would make sense for upstream to support, so I'm keeping it downstream until I can find reasonable ways to upstream it all. It would have been much easier to just upstream the whole thing from the start, however **LLVM is not a dumping ground for vendors**, so it would have been bad open-source citizenship to do that.

So:

- Libc++ welcomes contributions for adding support to new platforms -- it makes it more widely useful, which is awesome.
- Libc++ is not a dumping ground for arbitrary vendor-specific stuff -- there's too many vendors with too many use cases that require tweaking libc++, we couldn't realistically accommodate everyone even if we wanted to.
- Having some amount of downstream-only diff is normal. The key is to manage it to reduce downstream merge conflicts.

Also, the devil's in the details -- I am much less concerned by adding something self-contained like `zos_rename_dll_side_deck.sh` than tweaking the CMake itself, since we can basically forget about `zos_rename_dll_side_deck.sh` without harm, however we'll have to deal with the CMake complexity forever. Of course, upstreaming `zos_rename_dll_side_deck.sh` on its own doesn't make much sense -- I'm just trying to nuance my position.

FWIW, I think there might be ways to achieve some of what you want in a way that benefits everyone. Several vendors need to run post-install scripts or have slightly more complicated ways of building the library (see for example `libcxx/utils/ci/apple-install-libcxx.sh`, which I consider terrible tech debt for upstream and want to burn with fire). I suspect it may be possible to add customization points to the CMake build in a way that vendors can more easily configure it without adding tech debt for everyone, kind of like what we did for new style testing configurations (see for example `libcxx/test/configs/apple-libc++-backdeployment.cfg.in`).

================
Comment at: libcxx/include/__config_site.in:13
 #cmakedefine _LIBCPP_ABI_VERSION @_LIBCPP_ABI_VERSION@
+#if defined(__MVS__)
+#include <features.h> // for __NATIVE_ASCII_F
----------------
We don't put any logic inside `__config_site`. This logic should instead be implemented by setting a different namespace in the cache you use when building for EBCDIC/ASCII (you probably want two caches).

================
Comment at: libcxx/utils/libcxx/test/config.py:377
             self.cxx.link_flags += ['-lc++']
+            if self.target_info.is_zos():
+                self.cxx.link_flags += ['-lc++_a']
----------------
You should be using a new style config (see `libcxx/test/configs/`), not the legacy system. This is much easier to tweak for your exact needs.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118503