This is an archive of the discontinued LLVM Phabricator instance.

[sanitizers test cmake] Refactor the logic for compiling and generating the tests out into a function
ClosedPublic

Authored by george.karpenkov on Jul 31 2017, 3:01 PM.

Details

Summary

Most CMake configuration under compiler-rt/lib/*/tests have almost-the-same-but-not-quite functions of the form add_X_[unit]tests for compiling and running the tests.
Much of the logic is duplicated with minor variations across different sub-folders.
This can harm productivity for multiple reasons:

  • For newcomers, resulting CMake files are very large, hard to understand, and hide the intention of the code.
  • Changes for enabling certain architectures end up being unnecessarily large, as they get duplicated across multiple folders.
  • Adding new sub-projects requires more effort than it should, as a developer has to again copy-n-paste the configuration, and it's not even clear from which sub-project it should be copy-n-pasted.

With this change the logic of compile-and-generate-a-set-of-tests is extracted into a function, which hopefully makes writing and reading CMake much easier.
As a part of refactoring, I've changed affected macros to functions, as it's much easier to write "safer" CMake code using functions (if such thing exists).

Diff Detail

Repository
rL LLVM

Event Timeline

Removed unused function.

vitalybuka edited edge metadata.Jul 31 2017, 5:45 PM

Is possible to split into smaller patches?

cmake/Modules/AddCompilerRT.cmake
291 ↗(On Diff #109003)

Should arch be in the doc string as well.

312 ↗(On Diff #109003)

Use of closing parenthesis is inconsistent

342 ↗(On Diff #109003)

output_folder -> output_dir

@vitalybuka I could try if that's absolutely required.
At the end a single action is being done on multiple files: compile-and-generate functionality is extracted.

Applied review comments.

"ninja check-asan-dynamic" fails

[3/9] Linking CXX static library lib/libgtest.a
[4/9] cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-i386-inline-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
FAILED: projects/compiler-rt/lib/asan/tests/CMakeFiles/Asan-i386-inline-DynamicTest
cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-i386-inline-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/crt1.o(.text+0x20): error: undefined reference to 'main'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[5/9] cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-i386-with-calls-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
FAILED: projects/compiler-rt/lib/asan/tests/CMakeFiles/Asan-i386-with-calls-DynamicTest
cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-i386-with-calls-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/crt1.o(.text+0x20): error: undefined reference to 'main'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[6/9] cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-x86_64-with-calls-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
FAILED: projects/compiler-rt/lib/asan/tests/CMakeFiles/Asan-x86_64-with-calls-DynamicTest
cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-x86_64-with-calls-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/crt1.o(.text+0x20): error: undefined reference to 'main'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[7/9] cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-x86_64-inline-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
FAILED: projects/compiler-rt/lib/asan/tests/CMakeFiles/Asan-x86_64-inline-DynamicTest
cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-x86_64-inline-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/crt1.o(.text+0x20): error: undefined reference to 'main'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[8/9] Running lint check for sanitizer sources...

george.karpenkov marked 2 inline comments as done.Jul 31 2017, 5:55 PM

@vitalybuka I can branch out a few tiny things in separate patches (function to macro, moving flags up the file in tsan), but this will make the diff only slightly smaller.

cmake/Modules/AddCompilerRT.cmake
312 ↗(On Diff #109003)

Right. What would you prefer? There are no coding guidelines for CMake, and I don't think existing code is always consistent in this manner.
I was applying the following convention: keep on the same line for short argument list (for brevity),
move to it's own line for long list (useful for moving arguments around).

@vitalybuka

"ninja check-asan-dynamic" fails

thanks for the information, working on the fix now!
I wasn't aware that this target exists, it's not even run on ninja check-all.

@vitalybuka fixed ninja check-asan-dynamic, the argument order was wrong.

Another problem

[1/6] cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang ASAN_INST_TEST_OBJECTS.gtest-all.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_asm_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_globals_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_interface_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_internal_interface_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_oob_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_mem_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_str_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_test_main.cc.i386-with-calls.o -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-i386-with-calls-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
FAILED: projects/compiler-rt/lib/asan/tests/CMakeFiles/Asan-i386-with-calls-DynamicTest 
cd /build/projects/compiler-rt/lib/asan/tests && /build/./bin/clang ASAN_INST_TEST_OBJECTS.gtest-all.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_asm_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_globals_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_interface_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_internal_interface_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_oob_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_mem_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_str_test.cc.i386-with-calls.o ASAN_INST_TEST_OBJECTS.asan_test_main.cc.i386-with-calls.o -o /build/projects/compiler-rt/lib/asan/tests/dynamic/Asan-i386-with-calls-DynamicTest -fuse-ld=gold -Wl,-allow-shlib-undefined -g --driver-mode=g++ -fsanitize=address -shared-libasan -pthread
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.gtest-all.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_asm_test.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_globals_test.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_interface_test.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_internal_interface_test.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_test.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_oob_test.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_mem_test.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_str_test.cc.i386-with-calls.o: incompatible target
/usr/local/bin/ld.gold: error: ASAN_INST_TEST_OBJECTS.asan_test_main.cc.i386-with-calls.o: incompatible target
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/crt1.o(.text+0x20): error: undefined reference to 'main'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)

@vitalybuka thanks! Could you be a bit more explicit?
What ninja target are you running? How can one discover those (apart from knowing that they exist)?
I've run check-all and check-asan-dynamic.

that was ninja check-asan-dynamic again

approximate cmake command

cmake -GNinja -DLLVM_APPEND_VC_REV=OFF -DCMAKE_C_COMPILER=/build/out/build/bin/clang -DCMAKE_CXX_COMPILER=/build/out/build/bin/clang++ -DLLVM_TABLEGEN=/build/out/build/bin/llvm-tblgen -DCLANG_TABLEGEN=/build/out/build/bin/clang-tblgen -DLLVM_USE_LINKER=gold -DLLVM_ENABLE_LLD=OFF -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_WERROR=ON -DLLVM_PARALLEL_LINK_JOBS=10 -DCMAKE_INSTALL_RPATH_USE_LINK_PATH=ON -DLLVM_INCLUDE_GO_TESTS=OFF -DLLVM_CCACHE_BUILD=ON -DLLVM_ENABLE_LIBCXX=OFF -DCMAKE_C_FLAGS= -DCMAKE_CXX_FLAGS= -DCMAKE_EXE_LINKER_FLAGS= /build/llvm

added a dependency for dynamic tests

@vitalybuka can't reproduce. Can you remove caching from your config? (I think DLLVM_CCACHE_BUILD, maybe others).
With caching on, I get different, also strange, errors.

I simplified cmake command to something line this, still the same error.
cmake -GNinja -DCMAKE_C_COMPILER=/build/out/build/bin/clang -DCMAKE_CXX_COMPILER=/build/out/build/bin/clang++ -DCMAKE_BUILD_TYPE=Release -DLLVM_PARALLEL_LINK_JOBS=10 -DLLVM_ENABLE_LIBCXX=OFF -DCMAKE_C_FLAGS= -DCMAKE_CXX_FLAGS= -DCMAKE_EXE_LINKER_FLAGS= /build/llvm

I will try to debug the patch in the morning.

Would be nice to have unrelated changes like moving core around, switch from macro to function, renames or argument changes as separate patches.

lib/asan/tests/CMakeLists.txt
187 ↗(On Diff #110663)

This way it works as expected:

if(COMPILER_RT_ASAN_HAS_STATIC_RUNTIME)
    set(dynamic_test_name "Asan-${arch}${TEST_KIND}-Dynamic-Test")
    if(MSVC)

      # With the MSVC CRT, the choice between static and dynamic CRT is made at
      # compile time with a macro. Simulate the effect of passing /MD to clang-cl.
      set(ASAN_DYNAMIC_TEST_OBJECTS)
      generate_asan_tests(ASAN_DYNAMIC_TEST_OBJECTS
        AsanDynamicUnitTests "${dynamic_test_name}"
        SUBDIR "dynamic"
        CFLAGS ${ASAN_UNITTEST_INSTRUMENTED_CFLAGS} -D_MT -D_DLL
        SOURCES ${ASAN_INST_TEST_SOURCES}
        LINK_FLAGS ${ASAN_DYNAMIC_UNITTEST_INSTRUMENTED_LINK_FLAGS}
          -Wl,-nodefaultlib:libcmt,-defaultlib:msvcrt,-defaultlib:oldnames
        )
    else()

      # Otherwise, reuse ASAN_INST_TEST_OBJECTS.
      set(ASAN_INST_TEST_OBJECTS)
      generate_asan_tests(ASAN_INST_TEST_OBJECTS
        AsanDynamicUnitTests "${dynamic_test_name}"
        SUBDIR "dynamic"
        LINK_FLAGS ${ASAN_DYNAMIC_UNITTEST_INSTRUMENTED_LINK_FLAGS}
        SOURCES ${ASAN_INST_TEST_SOURCES}
        CFLAGS ${ASAN_UNITTEST_INSTRUMENTED_CFLAGS} ${TEST_CFLAGS})
    endif()
  endif()

@vitalybuka sorry I don't understand your patch: the whole point is not to recompile ASAN_INST_TEST_OBJECTS and to reuse them from the previous compilation.
If you reset it to empty, the test just becomes a no-OP from my understanding.
I can move out a change from function to macro if you wish so, but it's only one line. I don't see any other bits which could be easily factored out into a different review.

@vitalybuka found the difference: it seems, my machine couldn't compile to i386, and the multi-architecture pass was not tested. Testing now.

Somehow tests work for me, probably because "SOURCES ${ASAN_INST_TEST_SOURCES} "

I see now that it relinks tests every time. But I reverted to the master and it still relinks 4 tests every time. So if you are trying to fix
this, please keep it out of refactoring patches.

Also if goal to reuse some build results, have you noticed that error about incompatibility:
error: ASAN_INST_TEST_OBJECTS.gtest-all.cc.i386-with-calls.o: incompatible target
Do you run both i386 and x86_64?
On x86_64 linux it builds and tests both version by default. If you run just one, maybe this is the reason why you don't see error.
Is possible that you build just one arch?
To confirm this, how many tests do you see for check-asan-dynamic

  • Testing: 1638 tests, 48 threads --

Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
Testing Time: 42.23s

Expected Passes    : 1355
Unsupported Tests  : 283

It would be much easier to review refactoring do in something like:

  1. For each sanitizer create empty own generate_xsan_tests
  2. Use multiple patches to incrementally move code into that function until all sanitizers converge to the exactly the same code, similar to generate_compiler_rt_tests
  3. Replace duplicates with common generate_compiler_rt_tests

Somehow tests work for me

Oh right, you've renamed add_compiler_rt_tests to generate_compiler_rt_tests.
Yeah, then of course it would work.

Is possible that you build just one arch?

Yeah, my machine did not have 32bit version of libstdc++ installed, hence the difference in logic.

Also if goal to reuse some build results

Yeah, the goal is to follow the same logic as before, which was reusing logic.
Yes, I am trying to reproduce the error now.

It would be much easier to review refactoring do in something like

Right, I see.
The disadvantage is that each individual commit might not make much sense (would be needlessly increasing, rather than decreasing the complexity).
Can we try a less radical approach? I think I see some more stuff which could be moved out into other patches.

Can we try a less radical approach? I think I see some more stuff which could be moved out into other patches.

i leave it to your best judgement

vitalybuka added inline comments.Aug 15 2017, 1:18 PM
lib/asan/tests/CMakeLists.txt
188 ↗(On Diff #111223)

Could you please make it Dynamic-Test as it was before?

205 ↗(On Diff #111223)

Have you resolved the issue here?
Why not just use generate_asan_tests?

lib/asan/tests/CMakeLists.txt
205 ↗(On Diff #111223)

In the process, took me a long time to get 32-bit builds working.
We can just use generate_asan_tests, sure, would make my job easier. I thought it's important to make it faster? But I guess we don't call it on the default test pass.

vitalybuka added inline comments.Aug 15 2017, 1:39 PM
lib/asan/tests/CMakeLists.txt
205 ↗(On Diff #111223)

I'd like to ask again.
Making faster is awesome, but please separate refactorings and behavior changes.
We should check how it works on bots before optimizations.

lib/asan/tests/CMakeLists.txt
205 ↗(On Diff #111223)

but please separate refactorings and behavior changes

@vitalybuka exactly, that's why I'm re-using previous object files --- because the previous configuration did that as well.

Anyway, I think I've found the error, pushing the update now.

Move detecting architecture flag to add_compiler_rt_test.

@vitalybuka could you test again on your machine? I still couldn't get compiling to 32 bits fully working.

updated test name

george.karpenkov marked an inline comment as done.Aug 15 2017, 2:28 PM
vitalybuka added inline comments.Aug 15 2017, 3:48 PM
lib/asan/tests/CMakeLists.txt
205 ↗(On Diff #111223)

It was missing ${arch}?

lib/asan/tests/CMakeLists.txt
205 ↗(On Diff #111223)

arch-specific link flags should have been inserted in the function which did linking, not in the function which did compilation.

vitalybuka accepted this revision.Aug 15 2017, 3:51 PM
This revision is now accepted and ready to land.Aug 15 2017, 3:51 PM
This revision was automatically updated to reflect the committed changes.

Thanks.
Now we probably need to look at http://google.github.io/sanitizers/show_bots.html to check if number of test is still the same.

Uh, check-sanitizer does not work now.
It worked with previous snapshot of the patch.

@vitalybuka indeed can reproduce, my bad. Will rollback if I can't fix it within couple of minutes.

vitalybuka added inline comments.Aug 15 2017, 4:19 PM
cmake/Modules/AddCompilerRT.cmake
327 ↗(On Diff #111257)

That's because ${TEST_LINK_FLAGS} was removed.
Could this be a quick fix?