[PATCH] D10707: Part 1 of 2-part patch: Use a specified list of languages in cmake project() command.

Douglas Katzman dougk at google.com
Mon Jul 27 16:51:16 PDT 2015


dougk added inline comments.

================
Comment at: cmake/modules/HandleLLVMOptions.cmake:169
@@ -168,2 +168,3 @@
     set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${flag}" PARENT_SCOPE)
+    set(CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} ${flag}" PARENT_SCOPE)
   else()
----------------
rnk wrote:
> This looks like it adds assembler flags without checking if the assembler supports them. What flags need to come through here? -mcpu? I think the list of flags that we need to pass to the assembler is probably shorter than the list of flags we pass to the compiler, and we might want to special case things in that direction.
That point is in general valid - some flag might be an error for the assembler. But in practice, the only flag ever passed to add_flag_or_print_warning is '-fPIC' (in the same file, about 20 lines down).

In terms of deciding whether a flag is acceptable to the assembler, this is difficult.
There is no CheckASMSourceCompiles() function which would be the analogous thing to CheckCXXSourceCompiles, which is a cmake macro that tests whether any given flag will fail the compile step. The cmake documentation explicitly states that there is no test for whether the assembler "works".  Writing such a test might be possible by first compiling a trivial C program into assembly, then running the assembler with the additional flag to see whether that flag causes failure. But I think we don't need it (yet). The bottom line is that we have to assume that 'fPIC' is accepted by the assembler whenever it is acceptable to the CXX compiler.

There is also the question of whether other flags need to go into CMAKE_ASM_FLAGS that are not added specifically in add_flag_or_print_warning. Visual inspection suggests not. Pretty much all of the flags that are added into CMAKE_C_FLAGS and CMAKE_CXX_FLAGS are '-f' and '-W' options, which have no relevance to assembly source.


http://reviews.llvm.org/D10707







More information about the llvm-commits mailing list