This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Fix building unit tests on Darwin
ClosedPublic

Authored by beanz on Aug 19 2015, 4:04 PM.

Details

Summary

There are a number of issues with unit tests on Darwin. These patches address the following:

  • Unit tests should be passed -arch (-m32/-m64 isn't sufficient)
  • Unit tests should be passed ${DARWIN_osx_CFLAGS} because they're being built for OS X
  • Test architectures should be filtered based on base system capabilities (i.e. don't try running x86_64h tests on pre-haswell hardware).

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 32632.Aug 19 2015, 4:04 PM
beanz retitled this revision from to [CMake] Fix building unit tests on Darwin.
beanz updated this object.
beanz added reviewers: bogner, filcab, kubamracek.
beanz added a subscriber: llvm-commits.
filcab edited edge metadata.Aug 19 2015, 4:27 PM

Thanks a lot!

cmake/Modules/CompilerRTDarwinUtils.cmake
83 ↗(On Diff #32632)

Since this function is Darwin-specific, maybe we want to add the darwin_ prefix, to avoid name clashes.

test/asan/CMakeLists.txt
49 ↗(On Diff #32632)

Could this be done in get_target_flags_for_arch?

test/sanitizer_common/CMakeLists.txt
38 ↗(On Diff #32632)

Same as for test/asan/CMakeLists.txt.

beanz added a comment.Aug 19 2015, 5:08 PM

I'll send updated patches shortly. I'm testing them now.

cmake/Modules/CompilerRTDarwinUtils.cmake
83 ↗(On Diff #32632)

Sure.

test/asan/CMakeLists.txt
49 ↗(On Diff #32632)

Looking at this it does seem like get_target_flags_for_arch is only used when building tests, so yes. I originally didn't want to do that because I was populating Darwin OS-specific flags, not arch-specific flags.

beanz updated this revision to Diff 32643.Aug 19 2015, 5:24 PM
beanz edited edge metadata.

Fixing patches based on filcab's feedback.

beanz accepted this revision.Aug 20 2015, 10:32 AM
beanz added a reviewer: beanz.

Marking as accepted per bogner's email on llvm-commits. I will address his nit and commit shortly.

This revision is now accepted and ready to land.Aug 20 2015, 10:32 AM
This revision was automatically updated to reflect the committed changes.

It seems you left some conditions on the if that shouldn't be there. At least for now (when OS X is the only OS where we run unit tests).

cmake/config-ix.cmake
245 ↗(On Diff #32643)

Should we add the DARWIN_osx_CFLAGS here too, for now? That would minimize the number of places where you have to add Apple-specific conditions.
Eventually we might want to also run the tests for iOS, but for now, if you're on APPLE, then you know you're compiling for OS X. Might as well just add all the flags now.

lib/asan/tests/CMakeLists.txt
120 ↗(On Diff #32643)

Didn't you already add -arch ${arch} in get_target_flags_for_arch?

test/asan/CMakeLists.txt
45 ↗(On Diff #32643)

Why the NOT APPLE? Shouldn't you do the same for APPLE too?

test/sanitizer_common/CMakeLists.txt
34 ↗(On Diff #32643)

Same as test/asan/CMakeLists.txt.

filcab,

I've just committed r245582 to adapt to this feedback. Let me know if there are other concerns.

Thanks,
-Chris

cmake/config-ix.cmake
245 ↗(On Diff #32643)

For building tests DARWIN_osx_CFLAGS isn't needed, so I was trying to avoid more platform-specific hacking. Not really sure what this is going to look like yet once we fix it.

lib/asan/tests/CMakeLists.txt
120 ↗(On Diff #32643)

I think I can actually remove this whole block. I just missed it.

test/asan/CMakeLists.txt
45 ↗(On Diff #32643)

Yep, this is just another part of the patches I missed cleaning up. Will fix.