phosek added inline comments.
================ Comment at: clang/cmake/caches/Fuchsia-stage2.cmake:73 + +if(WIN32 OR (LINUX AND WINDOWS_SDK_DIR)) +#if(WINDOWS_SDK_DIR) ---------------- I'd drop this part altogether, let's build these anytime when `WINDOWS_SDK_DIR` is set. ================ Comment at: clang/cmake/caches/Fuchsia-stage2.cmake:96-99 + -Xclang + -ivfsoverlay + -Xclang + "${WINDOWS_SDK_DIR}/llvm-vfsoverlay.yaml" ---------------- I think we should introduce a new LLVM CMake option for setting this akin to `LLVM_WINSYSROOT` (for example `LLVM_VFSOVERLAY`). ================ Comment at: clang/cmake/caches/Fuchsia-stage2.cmake:100-101 + "${WINDOWS_SDK_DIR}/llvm-vfsoverlay.yaml" + /winsysroot + ${WINDOWS_SDK_DIR}) + string(REPLACE ";" " " CLANG_WINDOWS_CROSS_FLAGS "${CLANG_WINDOWS_CROSS_FLAGS}") ---------------- Can we instead set `BUILTINS_${target}_LLVM_WINSYSROOT` to `${WINDOWS_SDK_DIR}`? ================ Comment at: clang/cmake/caches/Fuchsia-stage2.cmake:105-108 + -libpath:"${WINDOWS_SDK_DIR}/VC/Tools/MSVC/14.34.31933/lib/x64" + -libpath:"${WINDOWS_SDK_DIR}/VC/Tools/MSVC/14.34.31933/atlmfc/lib/x64" + -libpath:"${WINDOWS_SDK_DIR}/Windows Kits/10/Lib/10.0.19041.0/ucrt/x64" + -libpath:"${WINDOWS_SDK_DIR}/Windows Kits/10/Lib/10.0.19041.0/um/x64") ---------------- These should be set automatically if you set `/winsysroot` (through `LLVM_WINSYSROOT`), see https://github.com/llvm/llvm-project/blob/a64846bee0bb4b4912c8cf6bf018ba5d892065d1/clang/lib/Driver/ToolChains/MSVC.cpp#L107. ================ Comment at: clang/cmake/caches/Fuchsia-stage2.cmake:144-145 + "${WINDOWS_SDK_DIR}/llvm-vfsoverlay.yaml" + /winsysroot + ${WINDOWS_SDK_DIR}) + string(REPLACE ";" " " CLANG_WINDOWS_CROSS_FLAGS "${CLANG_WINDOWS_CROSS_FLAGS}") ---------------- Can we instead set `RUNTIMES_${target}_LLVM_WINSYSROOT` to `${WINDOWS_SDK_DIR}`? ================ Comment at: llvm/cmake/modules/LLVMExternalProjectUtils.cmake:172 + -DCMAKE_ASM_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang-cl${CMAKE_EXECUTABLE_SUFFIX} + -DCMAKE_RC_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-rc + -DCMAKE_MT=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-mt ---------------- This should be in a separate block just like other tools below guarded by a `llvm-rc IN_LIST TOOLCHAIN_TOOLS` condition. ================ Comment at: llvm/cmake/modules/LLVMExternalProjectUtils.cmake:173 + -DCMAKE_RC_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-rc + -DCMAKE_MT=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-mt + ) ---------------- This should be in a separate block just like other tools below guarded by a `llvm-mt IN_LIST TOOLCHAIN_TOOLS` condition. ================ Comment at: llvm/runtimes/CMakeLists.txt:359 - if(NOT RUNTIMES_${name}_LLVM_USE_LINKER AND NOT RUNTIMES_${target}_LLVM_USE_LINKER) + if(NOT RUNTIMES_${name}_LLVM_USE_LINKER AND NOT RUNTIMES_${target}_LLVM_USE_LINKER AND NOT ${target} STREQUAL "x86_64-pc-windows-msvc") list(APPEND ${name}_extra_args -DLLVM_USE_LINKER=${LLVM_USE_LINKER}) ---------------- We should fix the build so that this isn't needed, it should be possible to use `LLVM_USE_LINKER=lld` when targeting Windows. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141738/new/ https://reviews.llvm.org/D141738 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits