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

[Zibi Sarbino via Phabricator via libcxx-commits]

zibi requested review of this revision.
zibi added a comment.

Louis, thank you for the review and providing some suggestions. However, I'm still not sure how to proceed see my inline comments.



================
Comment at: libcxx/CMakeLists.txt:472-476
 # 'LIBCXX_COMPILE_FLAGS' and 'LIBCXX_LINK_FLAGS'
 if(ZOS)
+  # Make sure EBCDIC encoding is used by default on z/OS.
   add_target_flags_if_supported("-fzos-le-char-mode=ebcdic")
 endif()
----------------
The `add_target_flags_if_supported("-fzos-le-char-mode=ebcdic")` will be removed since I found an alternative way.


================
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
----------------
ldionne wrote:
> 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.
I'm not entire sure how you envision this. You can not move the logic to `__config` since `#cmakedefine` has to be preprocessed by cmake. More importantly I think you asking for 2 separate builds which we want to avoid. See my other inline comment.


================
Comment at: libcxx/src/CMakeLists.txt:297
 
+# Build the ascii shared library for z/OS.
+if (LIBCXX_ASCII_ZOS_BUILD)
----------------
ldionne wrote:
> 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.
> 
Does current build infrastructure allow to reuse the previous build and build only c++? 

We build c++ only in both encoding modes and the rest of the libraries in EBCDIC. The c++ has dependency on rest of the libraries like c++_abi, unwinder, etc. We did not want to duplicated the entire library in both modes.

The first build would be the EBCDIC one and that is all good. However, the second ASCII build would throw away anything but c++_a. The most importantly issue would be how to make dependency to EBCDIC libraries from the 1st build rather then ASCII build from the 2nd one.




================
Comment at: libcxxabi/CMakeLists.txt:240-244
 if(ZOS)
+  # Make sure EBCDIC encoding is used by default on z/OS.
   add_target_flags_if_supported("-fzos-le-char-mode=ebcdic")
 endif()
 
----------------
The add_target_flags_if_supported("-fzos-le-char-mode=ebcdic") will be removed since I found an alternative way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118503