[test-suite] parameterization of llvm_{multi,single}source()

[tra]

Summary

This patch allows various compiler options as arguments to llvm_{multi,single}source().

Both now accept following optional arguments that are used in addition to
those specified via CFLAGS/CXXFLAGS/CPPFLAGS/LDFLAGS variables. This
allows adjusting build parameters for specific target without having to
modify global variables which makes build files somewhat less verbose.

In addition to global option, I've added two flags to further customize llvm
test target: LIBS and DEPS. This allows more control over targets that need
extra dependencies or need to link with shared libraries in non-standard directories.

• CFLAGS/CXXFLAGS/CPPFLAGS -- added to COMPILE_FLAGS property
• LDFLAGS -- added to LINK_LIBRARIES property
• LIBS -- applies target_link_libraries() which allows cmake to pass proper flags like rpath for shared libraries.
• DEPS -- applies add_dependencies()

This change allows reducing amount of boilerplate for most of the simple test cases.
Upcoming patch converts MultiSource/* build files to use this functionality.

[MatzeB]

I like this part of the patch (in fact I wanted to a similar thing myself at some point): I think it would be nice to llvm_multisource() to mostly behave like add_executable() so it would force you to mention the PROG name and allow you to specify sourcefiles behind that without an explicit PROG or SOURCES prefix. I can understand though that you may want to first transition to this to fixup the CMakeLists.txt files and only later remove the prefixes...

Adding CFLAGS, CPPFLAGS, LDFLAGS, ... as optional arguments does indeed reduce the boilerplate, however I am concerned that this is the wrong interface. If this becomes common usage we will need to add each new variable we add in the future to llvm_multisource() or we will have a strange disconnect between variable and named argument usage again. If someone adds new variants of llvm_multisource() for some reason he has to replicate all the parsing logic and pass those values through. Our usage in the llvm_multisource() case is now different from the usage of the llvm_singlesource() case now. Maybe we should rather aim for something like:
`llvm_multisource(foo)
llvm_add_cflags(foo ...)
llvm_add_cxxflags(foo ...)` and once we have that provide some convencience version that combines typical options:
llvm_add_flags(foo CFLAGS ... CXXFLAGS ...)

Simply using target_link_libraries(foo barlib) should be nearly as simple as adding a LIBS argument but has the added benefit that it is the documented way to do it in cmake and is obvious to people familiar with cmake. Similarily with DEPS. It seems like those two flags are not used in most cases, so I think it would be better if you provide a convenience macro around those for the cases that do need them. Something along the lines of:
`macro(llvm_multisource_dep_libs)

... parse DEPS, LIBS ...
llvm_multisource(${prog} ...)
target_link_libraries(${prog} ...)
add_dependencies(${prog} ...)

endmacro()`

[tra]

In D20221#433597, @MatzeB wrote:

Adding CFLAGS, CPPFLAGS, LDFLAGS, ... as optional arguments does indeed reduce the boilerplate, however I am concerned that this is the wrong interface. If this becomes common usage we will need to add each new variable we add in the future to llvm_multisource() or we will have a strange disconnect between variable and named argument usage again. If someone adds new variants of llvm_multisource() for some reason he has to replicate all the parsing logic and pass those values through.

This argument applies to passing these values via global variables -- both llvm_singlesource() and llvm_multisource() use specific variables to pass their values to specific cmake commands. Whole parsing code is pretty much mechanical conversion from global variable name to its local __ARG_xxx variant. While it is possible to get them out of sync, it's not going to be big deal to find a fix it if that happens.

Our usage in the llvm_multisource() case is now different from the usage of the llvm_singlesource() case now.

Not really. Parsing that matters only happens in test_suite_add_executable(). llvm_* just cherry-pick two args and pass through the rest. Those two args are only needed to deal with idiosyncrasies of current interface (current files use PROG and MAIN arguments in rather peculiar way). That can be fixed and then there will be no difference between llvm_singlesource and multisource parsing-wise.

Maybe we should rather aim for something like:
`llvm_multisource(foo)
llvm_add_cflags(foo ...)
llvm_add_cxxflags(foo ...)` and once we have that provide some convencience version that combines typical options:
llvm_add_flags(foo CFLAGS ... CXXFLAGS ...)

It may be an option, though I'm not convinced we need it.

Simply using target_link_libraries(foo barlib) should be nearly as simple as adding a LIBS argument but has the added benefit that it is the documented way to do it in cmake and is obvious to people familiar with cmake. Similarily with DEPS. It seems like those two flags are not used in most cases,

DEPS and LIBS I've added to be used in CUDA tests (D19434) where I need both extra build dependencies and libraries.
Using cmake API was rather cumbersome. Details are below.

so I think it would be better if you provide a convenience macro around those for the cases that do need them. Something along the lines of:
`macro(llvm_multisource_dep_libs)

... parse DEPS, LIBS ...
llvm_multisource(${prog} ...)
target_link_libraries(${prog} ...)
add_dependencies(${prog} ...)

endmacro()`

Ah, but ${prog} is *not* the target we create.

PROG is currently just something we use to check against blacklist of entities to skip.
Real target is result of get_unique_exe_name(executable ${_ARG_MAIN}) and we just rely on the fact that MAIN is either set to be ${PROG}.c which usually happens to be converted back to equivalent of ${PROG} by get_unique_exe_name() or, in case of llvm_single_source() we derive PROG from the name of the source file. In trivial cases ${PROG} == ${MAIN}.c, but that's not true if, for instance, we need to add unique prefix. So, your example above will work for *some* test cases, but not for all.

The problem is that it all happens under the hood and we, generally speaking, don't know the name of real target created by llvm_multisource() or llvm_singlesource() and thus can't use regular cmake API to augment rules to build that target.

Perhaps that's the first thing I should fix. Once we have a way to figure out the name of the target we've generated, we can do custom things outside of SingleMultiSource.cmake. It will also make it possible to make llvm_singlesource to be just a wrapper over llvm_multisource with a single source file and thus avoid divergence you're concerned about.

[MatzeB]

In D20221#433658, @tra wrote:

In D20221#433597, @MatzeB wrote:

Adding CFLAGS, CPPFLAGS, LDFLAGS, ... as optional arguments does indeed reduce the boilerplate, however I am concerned that this is the wrong interface. If this becomes common usage we will need to add each new variable we add in the future to llvm_multisource() or we will have a strange disconnect between variable and named argument usage again. If someone adds new variants of llvm_multisource() for some reason he has to replicate all the parsing logic and pass those values through.

This argument applies to passing these values via global variables -- both llvm_singlesource() and llvm_multisource() use specific variables to pass their values to specific cmake commands. Whole parsing code is pretty much mechanical conversion from global variable name to its local __ARG_xxx variant. While it is possible to get them out of sync, it's not going to be big deal to find a fix it if that happens.

Our usage in the llvm_multisource() case is now different from the usage of the llvm_singlesource() case now.

Not really. Parsing that matters only happens in test_suite_add_executable(). llvm_* just cherry-pick two args and pass through the rest. Those two args are only needed to deal with idiosyncrasies of current interface (current files use PROG and MAIN arguments in rather peculiar way). That can be fixed and then there will be no difference between llvm_singlesource and multisource parsing-wise.

Maybe we should rather aim for something like:
`llvm_multisource(foo)
llvm_add_cflags(foo ...)
llvm_add_cxxflags(foo ...)` and once we have that provide some convencience version that combines typical options:
llvm_add_flags(foo CFLAGS ... CXXFLAGS ...)

It may be an option, though I'm not convinced we need it.

I completely agree with you that the current global variable approach should be changed. It is hard to specify flags on a per-target basic, it is also hard to document/learn what variables are available and what their effect is. There is also no way to catch typos in variable names.

To give some context: I imagine that we will see macros like llvm_cuda(), llvm_halide(), llvm_googletest() popping up in the future. Those would basically call llvm_multisource() and perform some extra actions or add some default flags.

I can see pros and cons with either passing flags with additional function calls mentioning the target name or by adding additional named arguments to a single macro call:

• Functions are easy to discover and document. Named arguments may be interpreted by various functions which is harder to track and understand.
• Typos are easily diagnosed with function ("unknown function") with named arguments they will be parsed into the wrong list which may lead to hard to understand follow-up errors.
• The named argument name may clash with a compiler flag we want to use. This is probably so unlikely we can ignore it, but if it would ever happen it would be hard to work around.
• Named arguments give you a single list with all flags and settings which is easy to duplicate. This is useful to programmatically create multiple variants of a test.
• Named arguments force us to collect all flags/variables first and pass them along in a single call.
  • This can be nice because we have a single point to check for errors and consistency and are guaranteed to have no ordering problems.
  • It can also be annoying because it mostly enforces an order on you (no adding of a few flags afterwards).
• You need to type less with named arguments. The functions approach requires you to repeatedly mention the target name with each function call.
• The core cmake macros look more like the function approach.
• I can imagine work arounds for any of the problems mentioned above in both approaches (some simple and obvious some leading to ugly code).

I don't see any strong argument either way but have a weak tendency towards the function approach.

Simply using target_link_libraries(foo barlib) should be nearly as simple as adding a LIBS argument but has the added benefit that it is the documented way to do it in cmake and is obvious to people familiar with cmake. Similarily with DEPS. It seems like those two flags are not used in most cases,

DEPS and LIBS I've added to be used in CUDA tests (D19434) where I need both extra build dependencies and libraries.
Using cmake API was rather cumbersome. Details are below.

so I think it would be better if you provide a convenience macro around those for the cases that do need them. Something along the lines of:
`macro(llvm_multisource_dep_libs)

... parse DEPS, LIBS ...
llvm_multisource(${prog} ...)
target_link_libraries(${prog} ...)
add_dependencies(${prog} ...)

endmacro()`

Ah, but ${prog} is *not* the target we create.

PROG is currently just something we use to check against blacklist of entities to skip.
Real target is result of get_unique_exe_name(executable ${_ARG_MAIN}) and we just rely on the fact that MAIN is either set to be ${PROG}.c which usually happens to be converted back to equivalent of ${PROG} by get_unique_exe_name() or, in case of llvm_single_source() we derive PROG from the name of the source file. In trivial cases ${PROG} == ${MAIN}.c, but that's not true if, for instance, we need to add unique prefix. So, your example above will work for *some* test cases, but not for all.

The problem is that it all happens under the hood and we, generally speaking, don't know the name of real target created by llvm_multisource() or llvm_singlesource() and thus can't use regular cmake API to augment rules to build that target.

Perhaps that's the first thing I should fix. Once we have a way to figure out the name of the target we've generated, we can do custom things outside of SingleMultiSource.cmake. It will also make it possible to make llvm_singlesource to be just a wrapper over llvm_multisource with a single source file and thus avoid divergence you're concerned about.

I completely agree here, I would describe it as: llvm_multisource() should not exist and people should just directly call test_suite_add_executable() instead. (llvm_singlesource() is already a wrapper over that).

We really should have a way to know the target name. Maybe have a function that returns the target name when given a test name or maybe we can restrict the usage of llvm_target_prefix() to llvm_singlesource() so that at least you have a natural correspondence between test name and target name for all other cases...

[tra]

Rebased on top of recent HEAD.

[tra]

D20842 supersedes this patch.

cmake/modules/SingleMultiSource.cmake

 
 macro (test_suite_add_build_dependencies executable)
   if (NOT TEST_SUITE_USE_PERF)
     add_dependencies(${executable} timeit-target)
   endif()
   add_dependencies(${executable} timeit-host fpcmp-host)
 endmacro()
 
-macro(test_suite_add_executable name mainsource)
-  list(FIND PROGRAMS_TO_SKIP ${name} name_idx)
+macro(test_suite_add_executable)
+  cmake_parse_arguments(_ARG "" "PROG;MAIN" "SOURCES;CFLAGS;CPPFLAGS;CXXFLAGS;LDFLAGS;LIBS;DEPS" ${ARGN})
+  list(FIND PROGRAMS_TO_SKIP ${_ARG_PROG} name_idx)
   # Should we skip this?
   if(${name_idx} EQUAL -1)
-    get_unique_exe_name(executable ${mainsource})
-    add_executable(${executable} ${ARGN})
-    append_compile_flags(${executable} ${CFLAGS})
-    append_compile_flags(${executable} ${CPPFLAGS})
-    append_compile_flags(${executable} ${CXXFLAGS})
+    get_unique_exe_name(executable ${_ARG_MAIN})
+    add_executable(${executable} ${_ARG_SOURCES})
+    append_compile_flags(${executable} ${CFLAGS} ${_ARG_CFLAGS})
+    append_compile_flags(${executable} ${CPPFLAGS} ${_ARG_CPPFLAGS})
+    append_compile_flags(${executable} ${CXXFLAGS} ${_ARG_CXXFLAGS})
     # Note that we cannot use target_link_libraries() here because that one
     # only interprets inputs starting with '-' as flags.
-    append_link_flags(${executable} ${LDFLAGS})
+    append_link_flags(${executable} ${LDFLAGS} ${_ARG_LDFLAGS})
+    if (_ARG_LIBS)
+      target_link_libraries(${executable} ${_ARG_LIBS})
+    endif()
     set(executable_path ${CMAKE_CURRENT_BINARY_DIR}/${executable})
     if (TEST_SUITE_PROFILE_USE)
       append_compile_flags(${executable} -fprofile-instr-use=${executable_path}.profdata)
       append_link_flags(${executable} -fprofile-instr-use=${executable_path}.profdata)
     endif()
+    if (${_ARG_DEPS})
+      add_dependencies(${executable} ${_ARG_DEPS})
+    endif()
 
     set_property(GLOBAL APPEND PROPERTY TEST_SUITE_TARGETS ${executable})
 
     # Fall back to old style involving RUN_OPTIONS and STDIN_FILENAME if
     # llvm_test_run() was not called yet.
     if(NOT TESTSCRIPT)
-      llvm_test_traditional(${executable_path}.test ${executable_path} ${name})
+      llvm_test_traditional(${executable_path}.test ${executable_path} ${_ARG_PROG})
     else()
       llvm_add_test(${executable_path}.test ${executable_path})
     endif()
     test_suite_add_build_dependencies(${executable})
   endif()
 endmacro()
 
 # Configure the current directory as a SingleSource subdirectory - i.e. every
 # file in *.{c,cpp,cc} is treated as its own test.
 macro(llvm_singlesource)
   file(GLOB sources *.c *.cpp *.cc)
   foreach(source ${sources})
     # Find the pure name of the test
     string(REGEX REPLACE ".[cp]+$" "" path ${source})
     string(REGEX REPLACE ".*/" "" name ${path})
 
-    test_suite_add_executable(${name} ${source} ${source})
+    test_suite_add_executable(PROG "${name}" MAIN "${source}" SOURCES ${source} ${ARGN})
   endforeach()
 endmacro()
 
 # Configure the current directory as a MultiSource subdirectory - i.e. there is
 # one test and it consists of all sources in the directory (or a curated list,
 # if Source is defined).
 macro(llvm_multisource)
-  if(DEFINED Source)
-    set(sources ${Source})
+  cmake_parse_arguments(_ARG "" "PROG" "SOURCES" ${ARGN})
+  if(DEFINED Source OR _ARG_SOURCES)
+    set(sources ${Source} ${_ARG_SOURCES})
   else()
     file(GLOB sources *.c *.cpp *.cc)
   endif()
   list(LENGTH sources sources_len)
-
-  if(sources_len GREATER 0 AND DEFINED PROG)
+  # PROG passed as an argument has higher precedence.
+  if (_ARG_PROG)
+    set(_PROG ${_ARG_PROG})
+  elseif(DEFINED PROG)
+    set(_PROG ${PROG})
+  endif()
+  if(sources_len GREATER 0 AND _PROG)
     include_directories(${CMAKE_CURRENT_SOURCE_DIR})
     include_directories(${CMAKE_CURRENT_BINARY_DIR})
-    test_suite_add_executable(${PROG} "${PROG}.c" ${sources})
+    test_suite_add_executable(PROG ${_PROG} MAIN "${_PROG}.c" SOURCES ${sources} ${ARGN})
   endif()
 endmacro()