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

[Louis Dionne via Phabricator via libcxx-commits]

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

I left some comments. This still doesn't meet the bar since it hardcodes a bunch of z/OS specific knowledge in the build. However, I can see ways out of it, as explained in the comments.

In D118503#3646431 <https://reviews.llvm.org/D118503#3646431>, @zibi wrote:

> The CI is clean now, Can I have the approval or more request, Thx.

AFAICT we don't have z/OS CI set up anyway, so it wouldn't show up even if the patch didn't work. At least we do know it does not break other configurations, indeed.



================
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
----------------
SeanP wrote:
> ldionne wrote:
> > 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).
> Unless we are missing something, I don't think this kind of requirement is reasonable.  We need to be able to the distinguish between ebcdic/ascii with the installed headers.  If we don't put conditional logic into the `__config` header, we will be creating horribly complicated code (if it's even possible) in the compiler.  If we can't put conditional logic into `__config`, we will be required to ship two copies of the the libc++ headers (one where `__config` is set up for ascii and one where it is set up for ebcdic).  This is going to force the compiler to change the include dir for libc++ based on the the ascii/ebcdic option.  That is going to break the -isystem option.
> 
> We need one set of header files that defines `_LIBCPP_ABI_NAMESPACE` differently at compile time depending on if it is ascii or ebcdic.  This is the only way we can see doing this.  If there is a better way we are open to doing it that way.  The cache files don't work as they would result in two different set of headers.
> If we don't put conditional logic into the `__config` header

Logic inside `__config` is fine, but logic inside `__config_site` is not. This is adding logic inside `__config_site`.

We made changes to Clang to make it easy to ship libc++ headers as two things:

1. A per-target include directory that contains `__config_site` (and only that)
2. A target-agnostic `c++/v1` directory that contains all the other headers

Then, the driver selects the right target-specific directory so the right `__config_site` header is picked up. I believe you should be using that instead. If you did, it would actually be quite nice: there wouldn't be any specific logic here since you'd define a different `_LIBCPP_ABI_NAMESPACE` in each build of libc++ that you do (ASCII and non-ASCII), and that would result in two different `__config_site`s in different directories.


================
Comment at: libcxx/src/CMakeLists.txt:297
 
+# Build the ascii shared library for z/OS.
+if (LIBCXX_ASCII_ZOS_BUILD)
----------------
The only things that this target does differently from `cxx_shared` is pass `-fzos-le-char-mode=ascii`, set the output name to `c++_a` and add a post-build command.

Instead, the following would avoid hardcoding platform-specific knowledge in our build:
1. Build twice, with `CMAKE_CXX_COMPILE_FLAGS=-fzos-le-char-mode=ascii|ebcdic`. This would also remove the need for setting `add_target_flags_if_supported("-fzos-le-char-mode=ebcdic")` in `libcxxabi/CMakeLists.txt` 
2. Add a way to customize the output name of libc++ (by default it would be `c++`, but you'd set it to `c++_a` in your CMake cache)
3. Add your post-build step to `cxx_shared` under some option. This isn't clean, but I could refactor that eventually to provide a way to run arbitrary post-build steps more cleanly.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118503