[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 inline comments.
This revision now requires changes to proceed.


================
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}")
----------------
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?


================
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 "")
+
----------------
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.


================
Comment at: libcxx/cmake/caches/s390x-ibm-zos.cmake:25
+
+set(LLVM_EXTERNAL_UNWIND_SOURCE_DIR "$ENV{LLVM_BASE}/zos-unwind" CACHE PATH "The path to the external unwind source directory.")
----------------
Same here, this does not exist.


================
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.")
----------------
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")`?


================
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.")
----------------
This is not used anywhere. Is there a reason why it's in all the caches?


================
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()
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118503