Check build-type dependent variables for -flto as well.

[fhahn]

Summary

Currently we do not collect stats during the link step, if -flto is
added to the build-type dependent CMAKE_C/CXX_FLAGS variables. We should
check those variables as well.

[lebedev.ri]

Two thoughts:

1. Why is LDFLAGS being set conditionally, is there a warning otherwise?
2. I wonder, why it's {C,CXX,LD}FLAGS, and not CMAKE_*_FLAGS directly.

[fhahn]

In D61243#1481819, @lebedev.ri wrote:

Two thoughts:

1. Why is LDFLAGS being set conditionally, is there a warning otherwise?

I think it is set conditionally, because the selected linker exe may not support it. When doing LTO and saving stats, the assumption is that people are using clang for linking.

2. I wonder, why it's {C,CXX,LD}FLAGS, and not CMAKE_*_FLAGS directly.

Not sure, TBH.

[lebedev.ri]

In D61243#1484319, @fhahn wrote:

In D61243#1481819, @lebedev.ri wrote:

Two thoughts:

1. Why is LDFLAGS being set conditionally, is there a warning otherwise?

I think it is set conditionally, because the selected linker exe may not support it. When doing LTO and saving stats, the assumption is that people are using clang for linking.

Unless it is intentionally overridden along the way by the user, CMake is always using the compiler as the linker.

2. I wonder, why it's {C,CXX,LD}FLAGS, and not CMAKE_*_FLAGS directly.

Not sure, TBH.

[lebedev.ri]

https://github.com/Kitware/CMake/blob/c4b4d8b3a67718e29edb5676273e528dab566672/Modules/CMakeCXXInformation.cmake#L268-L271
so i'd really recommend to try to always set it, and see if that works

[fhahn]

I just realised that this patch was still around!

In D61243#1484435, @lebedev.ri wrote:

In D61243#1484319, @fhahn wrote:

In D61243#1481819, @lebedev.ri wrote:

Two thoughts:

1. Why is LDFLAGS being set conditionally, is there a warning otherwise?

I think it is set conditionally, because the selected linker exe may not support it. When doing LTO and saving stats, the assumption is that people are using clang for linking.

Unless it is intentionally overridden along the way by the user, CMake is always using the compiler as the linker.

That makes sense, thanks! But I think moving away from CFLAGS, CXXFLAGS and LDFLAGS is an orthogonal change to this one and there potentially are additional things that can go wrong there. Do you think that and the change as is makes sense?

[lebedev.ri]

In D61243#1717083, @fhahn wrote:

I just realised that this patch was still around!

In D61243#1484435, @lebedev.ri wrote:

In D61243#1484319, @fhahn wrote:

In D61243#1481819, @lebedev.ri wrote:

Two thoughts:

1. Why is LDFLAGS being set conditionally, is there a warning otherwise?

I think it is set conditionally, because the selected linker exe may not support it. When doing LTO and saving stats, the assumption is that people are using clang for linking.

Unless it is intentionally overridden along the way by the user, CMake is always using the compiler as the linker.

That makes sense, thanks! But I think moving away from CFLAGS, CXXFLAGS and LDFLAGS is an orthogonal change to this one and there potentially are additional things that can go wrong there. Do you think that and the change as is makes sense?

I don't think the change as-is is bad. Though i'm not sure it will catch everything - it can be set in subdirectory too.
Have you checked what happens if you do list(APPEND LDFLAGS -save-stats=obj) unconditionally?

[MatzeB]

I wonder, why it's {C,CXX,LD}FLAGS, and not CMAKE_*_FLAGS directly.

Not sure, TBH.

As far as I know the idea is that CFLAGS/CXXFLAGS/LDFLAGS are maintained as cmake lists so you can do stuff like list(FILTER on them. CMAKE_xxx_FLAGS are strings so it's harder to do it there.

The desire to have them as lists stems from the fact that the first version of the test-suite cmake files were mostly created by an automatic script reading the Makefiles needing to translate stuff like $(filter -lstdc++,$(LDFLAGS))...

So no strong reason, but will be annoying to change now.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

CMakeLists.txt

if(TEST_SUITE_COLLECT_COMPILE_TIME)
   set(CMAKE_CXX_COMPILE_OBJECT "${CMAKE_BINARY_DIR}/tools/timeit --summary <OBJECT>.time ${CMAKE_CXX_COMPILE_OBJECT}")
   set(CMAKE_C_LINK_EXECUTABLE "${CMAKE_BINARY_DIR}/tools/timeit --summary <TARGET>.link.time ${CMAKE_C_LINK_EXECUTABLE}")
   set(CMAKE_CXX_LINK_EXECUTABLE "${CMAKE_BINARY_DIR}/tools/timeit --summary <TARGET>.link.time ${CMAKE_CXX_LINK_EXECUTABLE}")
 endif()
 
 option(TEST_SUITE_BENCHMARKING_ONLY "Only run the benchmarking only subset" OFF)
 
 option(TEST_SUITE_COLLECT_STATS "Collect LLVM statistics" OFF)
+string(TOUPPER ${CMAKE_BUILD_TYPE} CMAKE_BUILD_TYPE_UPPER)
 if(TEST_SUITE_COLLECT_STATS)
   list(APPEND CFLAGS -save-stats=obj)
   list(APPEND CXXFLAGS -save-stats=obj)
   # Collect stats for LTO step too.
-  if (${CMAKE_C_FLAGS} MATCHES ".*-flto.*" AND
-      ${CMAKE_CXX_FLAGS} MATCHES ".*-flto.*")
+  if ("${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${CMAKE_BUILD_TYPE_UPPER}}" MATCHES ".*-flto.*" AND
+      "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${CMAKE_BUILD_TYPE_UPPER}}" MATCHES ".*-flto.*")
       list(APPEND LDFLAGS -save-stats=obj)
   endif()
 endif()
 
 # Detect and include subdirectories
 # This allows to: Place additional test-suites into the toplevel test-suite
 # directory where they will be picked up automatically. Alternatively you may
 # manually specify directories to include test-suites at external locations
 # and to leave out some of the default ones.