Skip to content

[libc] Include CheckCXXCompilerFlag when checking compiler features #118862

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

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

RossComputerGuy
Copy link
Member

Includes CheckCXXCompilerFlag so when building LLVM libc is built standalone, it actually works and doesn't complain about check_cxx_compiler_flag not being defined.

@llvmbot llvmbot added the libc label Dec 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2024

@llvm/pr-subscribers-libc

Author: Tristan Ross (RossComputerGuy)

Changes

Includes CheckCXXCompilerFlag so when building LLVM libc is built standalone, it actually works and doesn't complain about check_cxx_compiler_flag not being defined.


Full diff: https://github.com/llvm/llvm-project/pull/118862.diff

1 Files Affected:

  • (modified) libc/cmake/modules/CheckCompilerFeatures.cmake (+2)
diff --git a/libc/cmake/modules/CheckCompilerFeatures.cmake b/libc/cmake/modules/CheckCompilerFeatures.cmake
index 862c7ecbd7fdf4..85e1cabd1483e3 100644
--- a/libc/cmake/modules/CheckCompilerFeatures.cmake
+++ b/libc/cmake/modules/CheckCompilerFeatures.cmake
@@ -1,3 +1,5 @@
+include(CheckCXXCompilerFlag)
+
 # ------------------------------------------------------------------------------
 # Compiler features definition and flags
 # ------------------------------------------------------------------------------

Comment on lines 1 to 2
include(CheckCXXCompilerFlag)

Copy link
Contributor

Choose a reason for hiding this comment

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

Move below the header comments.

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

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

This should probably just go at the top level since it's used here

startup/linux/CMakeLists.txt
77:check_cxx_compiler_flag("-r" LIBC_LINKER_SUPPORTS_RELOCATABLE)

cmake/modules/CheckCompilerFeatures.cmake
136:check_cxx_compiler_flag("-ftrivial-auto-var-init=pattern" LIBC_CC_SUPPORTS_PATTERN_INIT)
139:check_cxx_compiler_flag("-nostdlib++" LIBC_CC_SUPPORTS_NOSTDLIBPP)
142:check_cxx_compiler_flag("-nostdlibinc" LIBC_CC_SUPPORTS_NOSTDLIBINC)

cmake/modules/prepare_libc_gpu_build.cmake
44:  check_cxx_compiler_flag(-mcpu=native PLATFORM_HAS_GPU)
46:  check_cxx_compiler_flag(-march=native PLATFORM_HAS_GPU)

@RossComputerGuy RossComputerGuy force-pushed the feat/llvm-libc-standalone branch from 33ec79a to 0984c89 Compare December 5, 2024 19:36
@RossComputerGuy
Copy link
Member Author

This should probably just go at the top level

Oh, alright. I grepped and had found that it was common to include in cmake level.

@RossComputerGuy RossComputerGuy force-pushed the feat/llvm-libc-standalone branch from 0984c89 to 84e0dde Compare December 5, 2024 19:41
@RossComputerGuy
Copy link
Member Author

I've moved it to the top level for libc.

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

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

nit. can you put it next to the include on line 8.

@RossComputerGuy RossComputerGuy force-pushed the feat/llvm-libc-standalone branch from 84e0dde to 989c7db Compare December 5, 2024 19:44
@RossComputerGuy
Copy link
Member Author

T'is done, thank you for the quick review.

Copy link
Contributor

@jhuber6 jhuber6 left a comment

Choose a reason for hiding this comment

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

Thanks

@jhuber6 jhuber6 merged commit 3a7d1b5 into llvm:main Dec 5, 2024
7 checks passed
@RossComputerGuy RossComputerGuy deleted the feat/llvm-libc-standalone branch December 5, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants