Page MenuHomePhabricator

[compiler-rt][builtins] CTest to execute builtins testsuite
Needs ReviewPublic

Authored by farazs on Jun 23 2014, 1:31 AM.

Details

Summary

There doesn't seem to be a defined procedure for running the builtins testsuite. There is a 'test' shell script in test/builtins/Unit but it seems to be outdated and specific to darwin with hard-coded paths.

Comments in test/CMakeLists.txt suggest that builtins and BlockRunTime testsuites have not yet been ported to lit framework. Attached is a rudimentary CTest setup to run the unit tests in builtins, as a stop-gap until the test
sources can be modified for lit.

The new target to trigger test run is `check-builtins'. It is intended to stop after building tests for cross-builds, else build and execute for native builds. Hope it is useful!

Diff Detail

Event Timeline

farazs updated this revision to Diff 10740.Jun 23 2014, 1:31 AM
farazs retitled this revision from to [compiler-rt][builtins] CTest to execute builtins testsuite.
farazs updated this object.
farazs edited the test plan for this revision. (Show Details)
farazs added reviewers: samsonov, howard.hinnant.
farazs set the repository for this revision to rL LLVM.
farazs added subscribers: Unknown Object (MLST), dsanders.
sdkie added a subscriber: sdkie.Jun 23 2014, 3:04 AM
samsonov edited edge metadata.Jul 1 2014, 5:13 PM

Sorry for delay, I haven't yet had a chance to review this. IIRC the rest of LLVM regression test suites doesn't really work with CTest, though. Not sure if it's a good temporary solution. Anyway, will provide more comments later.

Once again, sorry for delay with reviewing this.

test/builtins/CMakeLists.txt
1

Please specify somewhere that you're actually using a host compiler to build these tests, and link them against a just-built builtins library.

184

${COMPILER_RT_SOURCE_DIR}/lib/builtins

187

Is it picked up by CMake automatically?

193

please check COMPILER_RT_CAN_EXECUTE_TESTS here instead - we don't need to add targets otherwise.

196

consider using get_filename_component instead.

209

Use ${COMPILER_RT_LIBRARY_OUTPUT_DIR} instead.

214

Why not LINK_FLAGS?

I don't mind the delay, please take your time. This is not exactly high priority.
Thanks for your comments & corrections, I agree with most. Few clarifications below.

test/builtins/CMakeLists.txt
187

No. Gets picked on line 199 as ${{test}_EXTRA}

193

It is checked below for add_test(line 202). My plan was to be able to cross-build the test-suite even when we can't execute it. The dev environment is usually much faster and I think it might be useful to fix all build issues before moving to the target for execution.

If this doesn't seem like a good idea, can we just remove the check from this file and modify the upper test/CMakeLists.txt instead? It already has a COMPILER_RT_CAN_EXECUTE_TESTS block. Enclosing add_subdirectory in that block should give the desired effect and be consistent with the way sanitizer tests are controlled.

214

No hard reason - just following cmake docs. Linkers are known to be finicky about the order of command-line options. My guess is, to construct a correct invocation, cmake should be able to distinguish libraries from all other linker options.

FWIW, I agree that it would bring more benefit to just port these tests to lit. The RUN-line for them would be similar and trivial (smth. like RUN: %clang %s -O2 -o %t && %t), and would allow us to test just-built Clang, which seems like the right thing to do (we'll also automatically test that Clang driver appropriately links in builtins library). RUN-lines are also suited to the special cases like "compile this file with -fnested-functions").

test/builtins/CMakeLists.txt
193

Yes, the latter sounds good.

This builds the unit tests along with the library.
We want to build the unit tests when invoked with "check-builtins". Isn't it?

I am writing the lit cfg files for test/builtins and will upload it when I am done testing it.

In D4251#10, @sgundapa wrote:

We want to build the unit tests when invoked with "check-builtins". Isn't it?

That is exactly what this patch does.

I am writing the lit cfg files for test/builtins and will upload it when I am done testing it.

Great! In that case I won't bother doing a second revision. Should I mark this as abandoned?

You can mark this as abandoned once I push mine.
Sounds good ?

--Sumanth G

Hi Sumanth,

Any update on this?

Thanks,
Jaydeep