Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove -fsized-deallocation when build with clang-cl. #186

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jackie9527
Copy link
Contributor

Build o3de with VS2022 and clang-cl, clang-cl doesn't support -fsized-deallocation.

clang-cl, as follows:

clang --version
clang version 15.0.1
Target: x86_64-pc-windows-msvc
Thread model: posix

Script as follows:

set SRC_ROOT=E:\CODE\o3de
cmake -B %SRC_ROOT%\build\windows_clang ^
    -S %SRC_ROOT% ^
    -T ClangCl ^
    -DLY_UNITY_BUILD=ON ^
    -DLY_3RDPARTY_PATH=D:\CODE\o3de\3rd-packages ^
    -DLY_PROJECTS=AutomatedTesting

@lemonade-dm lemonade-dm requested review from a team and spham-amzn March 8, 2023 02:14
Comment on lines 21 to 23
if(NOT MSVC)
target_compile_options(3rdParty::pybind11 INTERFACE -fsized-deallocation)
endif()
Copy link
Contributor

@lemonade-dm lemonade-dm Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSVC is the compiler, not the OS of Windows.

So this if condition is always TRUE.
This needs check that the CMAKE_SYSTEM_NAME is not "Windows"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows platform, build o3de within vcvarsall.bat

  • using clang-cl, as follows:
cmake -B %SRC_ROOT%\build\windows_clang ^
    -S %SRC_ROOT% ^
    -T ClangCl ^
    ... // something else

CMAKE_CXX_COMPILER_ID is Clang and MSVC is not set.

  • using cl, as follows:
cmake -B %SRC_ROOT%\build\windows_clang ^
    -S %SRC_ROOT% ^
    -G "Visual Studio 17" ^
    ... // something else

MSVC is set.

As said, clang-cl shipped with VS2022 dose not support -fsized-deallocation.
When building o3de using clang-cl, clang-cl complains unknown options about -fsized-deallocation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows platform, build o3de within vcvarsall.bat

* using clang-cl, as follows:
cmake -B %SRC_ROOT%\build\windows_clang ^
    -S %SRC_ROOT% ^
    -T ClangCl ^
    ... // something else

CMAKE_CXX_COMPILER_ID is Clang and MSVC is not set.

If MSVC is not set then the check of NOT MSVC would succeed and add the -fsized-deallocation option to clang-cl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I made a mistake.

using clang-cl, as follows, CMAKE_CXX_COMPILER_ID is Clang and MSVC is set.

cmake -B %SRC_ROOT%\build\windows_clang ^
    -S %SRC_ROOT% ^
    -T ClangCl ^
    ... // something else

Copy link
Contributor

@lemonade-dm lemonade-dm Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using Windows clang++ and clangcompilers with CMake, it doesn't set the MSVC option. Only the Windowsclang-cl` compiler sets the MSVC option

Checking against OS of Windows might be safer via comparing the CMAKE_SYSTEM_NAME variable against the string of "Windows".

CMakeLists.txt

cmake_minimum_required(VERSION 3.21)

project(test_project C CXX)

set(CMAKE_CXX_STANDARD 17)

add_executable(test_project src/main.cpp)
target_compile_options(test_project PRIVATE "-fsized-deallocation")

install(TARGETS test_project)

message(STATUS "CMAKE_CXX_COMPILER_ID is ${CMAKE_CXX_COMPILER_ID}")
message(STATUS "The MSVC option is set to ${MSVC}")

cmd

C:\Users\lumberyard-employee-dm\source\repos\test_project>cmake -B build\windows-clang -S . -G"Ninja Multi-Config" -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
-- The C compiler identification is Clang 15.0.2 with GNU-like command-line
-- The CXX compiler identification is Clang 15.0.2 with GNU-like command-line
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/LLVM/bin/clang.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/LLVM/bin/clang++.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- CMAKE_CXX_COMPILER_ID is Clang
-- The MSVC option is set to
-- Configuring done
-- Generating done
-- Build files have been written to: C:/Users/lumberyard-employee-dm/source/repos/test_project/build/windows-clang

C:\Users\lumberyard-employee-dm\source\repos\test_project>cmake -B build\windows-clang -S . -G"Ninja Multi-Config" -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl
-- The C compiler identification is Clang 15.0.2 with MSVC-like command-line
-- The CXX compiler identification is Clang 15.0.2 with MSVC-like command-line
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/LLVM/bin/clang-cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/LLVM/bin/clang-cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- CMAKE_CXX_COMPILER_ID is Clang
-- The MSVC option is set to 1
-- Configuring done
-- Generating done
-- Build files have been written to: C:/Users/lumberyard-employee-dm/source/repos/test_project/build/windows-clang

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the clang++.exe and clang.exe on Windows supports the -fsized-allocation option.
It is just the clang-cl executable that doesn't support the option.

C:\Users\lumberyard-employee-dm\source\repos\test_project>cmake --build build\windows-clang
[1/2] Building CXX object CMakeFiles\test_project.dir\Debug\src\main.cpp.obj
clang-cl: warning: unknown argument ignored in clang-cl: '-fsized-deallocation' [-Wunknown-argument]
[2/2] Linking CXX executable Debug\test_project.exe

C:\Users\lumberyard-employee-dm\source\repos\test_project>cmake -B build\windows-clang -S . -G"Ninja Multi-Config" -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++
-- The C compiler identification is Clang 15.0.2 with GNU-like command-line
-- The CXX compiler identification is Clang 15.0.2 with GNU-like command-line
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/LLVM/bin/clang.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/LLVM/bin/clang++.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- CMAKE_CXX_COMPILER_ID is Clang
-- The MSVC option is set to
-- Configuring done
-- Generating done
-- Build files have been written to: C:/Users/lumberyard-employee-dm/source/repos/test_project/build/windows-clang

C:\Users\lumberyard-employee-dm\source\repos\test_project>cmake --build build\windows-clang
[2/2] Linking CXX executable Debug\test_project.exe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, indeed.

@Jackie9527 Jackie9527 requested review from lemonade-dm and removed request for spham-amzn March 9, 2023 09:01
Comment on lines 20 to 24
if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
target_compile_options(3rdParty::pybind11 INTERFACE -fsized-deallocation)
if(NOT MSVC)
target_compile_options(3rdParty::pybind11 INTERFACE -fsized-deallocation)
endif()
endif()
Copy link

@burelc-amzn burelc-amzn Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should enable the associated flag on MSVC?

Suggested change
if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
target_compile_options(3rdParty::pybind11 INTERFACE -fsized-deallocation)
if(NOT MSVC)
target_compile_options(3rdParty::pybind11 INTERFACE -fsized-deallocation)
endif()
endif()
if (MSVC)
target_compile_options(3rdParty::pybind11 INTERFACE /Zc:sizedDealloc)
elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
target_compile_options(3rdParty::pybind11 INTERFACE -fsized-deallocation)
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Signed-off-by: Jackie9527 <80555200+Jackie9527@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants