This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [cmake] [asan] Remove unnecessary gtest dep from dynamic tests
ClosedPublic

Authored by mgorny on Oct 12 2017, 1:50 AM.

Details

Summary

Remove the redundant dependency on 'gtest' target from the dynamic tests
in non-MSVC environment. The tests reuse compiled objects
from ASAN_INST_TEST_OBJECTS, and therefore they have been built against
gtest already.

This both fixes the spurious dependency on 'gtest' target that breaks
stand-alone builds, and brings the dynamic tests more in line with
regular tests which do not pass this dependency
to add_compiler_rt_test() through generate_compiler_rt_tests().

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Oct 12 2017, 1:50 AM
george.karpenkov requested changes to this revision.Oct 12 2017, 10:15 AM

@mgorny so this one actually changes semantics.
The previous one reused generated object files, and with the change it will start recompiling all the tests again.
(and actually being able to export object files has added quite a bit of complexity to all these compiler-rt macros).

The current code also looks weird, because it depends on ASAN_INST_TEST_OBJECTS, but will actually recompile and export them.

I think a better fix would be to change add_compiler_rt_test to accept two kinds of dependencies (probably again DEPS and COMPILE_DEPS? I know the name is not very descriptive, but I've kept it from the previous version).
It already checks that variable to conditionally add clang to dependencies.

This revision now requires changes to proceed.Oct 12 2017, 10:15 AM
mgorny updated this revision to Diff 118826.Oct 12 2017, 12:31 PM
mgorny edited edge metadata.
mgorny retitled this revision from [compiler-rt] [cmake] [asan] Reuse generate_asan_tests for dynamic tests to [compiler-rt] [cmake] [asan] Remove unnecessary gtest dep from dynamic tests.
mgorny edited the summary of this revision. (Show Details)

Ok, here's another idea. Since the objects have been compiled already (and they explicitly depend on gtest via their own deps), let's just remove the extraneous gtest dep.

george.karpenkov accepted this revision.Oct 12 2017, 1:19 PM

LGTM, provided tests pass both in standalone and "normal" modes.

This revision is now accepted and ready to land.Oct 12 2017, 1:19 PM
This revision was automatically updated to reflect the committed changes.