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

[Zibi Sarbino via Phabricator via libcxx-commits]

zibi marked an inline comment as done.
zibi added a comment.

answering comments and some questions



================
Comment at: libcxx/CMakeLists.txt:469-471
+  add_target_flags_if_supported("-DLIBCXX_BUILDING_LIBCXXABI")
+  add_target_flags_if_supported("-fzos-le-char-mode=ascii")
+  add_target_flags_if_supported("-I../${CMAKE_CXX_COMPILER_TARGET}/${LIBCXX_INSTALL_INCLUDE_DIR}")
----------------
ldionne wrote:
> Instead, let's add an option like `LIBCXX_ADDITIONAL_COMPILE_FLAGS` that we can use to pass those compiler flags from the cache. I'm not a huge fan of doing that, but I do think it's a useful backdoor to have.
> 
> However, even with `LIBCXX_ADDITIONAL_COMPILE_FLAGS`, I don't understand why `-I../${CMAKE_CXX_COMPILER_TARGET}/${LIBCXX_INSTALL_INCLUDE_DIR}` is needed. What purpose does it serve?
The -I.. is needed because we build only cxx_shared as ASCII and need to point to headers cxx-headers and cxxabi-headers from the full build we do in EBCDIC. 


================
Comment at: libcxx/cmake/caches/s390x-ibm-zos-ascii.cmake:15-16
+
+set(LLVM_ENABLE_ASSERTIONS ON CACHE BOOL "")
+set(LLVM_ENABLE_WERROR ON CACHE BOOL "")
+
----------------
ldionne wrote:
> Why are we setting `LLVM_FOO` variables in the CMake cache for libc++? Unless I am mistaken, I think neither of those two cache values will be picked up by anything in the runtimes build, and so they serve no purpose AFAICT.
Yes, you are right I will remove them.


================
Comment at: libcxx/cmake/caches/s390x32-ibm-zos-ascii.cmake:22
+set(LIBCXX_DLL_NAME CRTEHCXS CACHE STRING "")
+set(LIBCXX_SHARED_OUTPUT_NAME "c++_a" CACHE PATH
+    "Output name for the shared libc++ runtime library.")
----------------
ldionne wrote:
> I don't see this used anywhere. Should this be defined in `CMakeLists.txt` and then overriden here only with `set(LIBCXX_SHARED_OUTPUT_NAME "c++_a")`?
Yes, this is how I have implemented it in my local branch. For some reason it was not included in this patch. I will correct this.


================
Comment at: libcxx/cmake/caches/s390x32-ibm-zos-ascii.cmake:24
+    "Output name for the shared libc++ runtime library.")
+set(LIBCXX_EXCEPTION_ZOS_BUILD OFF CACHE BOOL
+    "Build libcxx exception library on z/OS as standalone.")
----------------
ldionne wrote:
> This is not used anywhere. Is there a reason why it's in all the caches?
This is needed for the exception library which we created by splitting cxx which is not included in this patch but it will be included in separate patch. The reason I'm keeping it here so I can easy verify current patch on my local branch.


================
Comment at: libcxx/src/CMakeLists.txt:203-204
+  if (NOT LIBCXX_ASCII_ZOS_BUILD)
+    target_link_libraries(cxx_shared PUBLIC cxx-headers
+                                     PRIVATE ${LIBCXX_LIBRARIES})
+  endif()
----------------
ldionne wrote:
> Why do you avoid linking `cxx_shared` against its dependencies in the z/OS build? Is it because you disable building the headers? I don't think this should be z/OS specific, this should probably be supported out of the box instead.
Yes we disable everything including headers when building `cxx_shared` in ASCII and want to link with dependencies from the `full` separate build done in EBCDIC. This is why we need -I and -Wl,<side-deck>.x options.

What do you mean out of the box. AFAIK, nobody builds only libcxx and depend on previous builds on its dependencies. Maybe I should have another variant of `LIBCXX_CXX_ABI`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118503