[PATCH] InstrProf: Add initial compiler-rt test

[Duncan P. N. Exon Smith]

Thanks for your review -- this was really helpful.  New patch attached
that addresses most of your comments (exceptions below).

On 2014 Mar 28, at 09:02, Alexey Samsonov <samsonov at google.com> wrote:

> Hi Duncan,
> 
> On Fri, Mar 28, 2014 at 8:21 AM, Kostya Serebryany <kcc at google.com> wrote:
> +samsonov
> 
> 
> On Fri, Mar 28, 2014 at 3:39 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> This patch adds the infrastructure for compiler-rt tests for instrumented profiling.
> It also adds a single very simple test.
> 
> I’m a little lost with lit and cmake; could someone who knows better have a look to
> make sure I have the right magic?
> 
> I copied and modified the logic from the test/asan/ directory.
> 
> There are two things that don’t (or probably don’t) work.
> 
>  1. You cannot call:
> 
>         $ cd llvm/projects/compiler-rt
>         $ path/to/llvm-lit test/profile
> 
>     The other compiler-rt tests seem to have the same defect, and I’m not sure how
>     to fix it.
> 
>     Nevertheless, using cmake+ninja, you can call:
> 
>         $ ninja check-profile
> 
>  2. I didn’t do anything with configure+make, since the other tests in compiler-rt
>     didn’t have a model I could copy.  So the tests probably aren’t registered
>     there.
> 
> Thanks for any pointers!
> 
> Duncan
> 
> 
> Could you please use phabricator (http://llvm.org/docs/Phabricator.html) for sending patches?

I'm continuing this review in the same thread to keep things simple.  I'll
try to remember your preference next time!

> 
> A few comments:
> 
> +
> +# Tests need to run generated executables.
> +if config.host_triple != config.target_triple:
> +  config.unsupported = True
> 
> This is not needed - if tests target different platform, COMPILER_RT_CAN_EXECUTE_TESTS should be OFF,
> and the test suite wouldn't be added.
> 
> 
> +set(PROFILE_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
> +
> +set(PROFILE_TESTSUITES)
> 
> ^^ Looks like this is not used. Please delete.
> 
> +
> +set(PROFILE_TEST_DEPS)
> +if(NOT COMPILER_RT_STANDALONE_BUILD)
> +  list(APPEND PROFILE_TEST_DEPS profile)
> 
> You also need to add dependency on clang, and likely clang-headers and llvm-profdata.
> 
> +endif()
> 
> 
> +
> +# Add clang substitutions.
> +config.substitutions.append( ("%clang ", config.clang + " ") )
> +config.substitutions.append( ("%clang_profgen ", config.clang + " -fprofile-instr-generate ") )
> +config.substitutions.append( ("%clang_profuse=", config.clang + " -fprofile-instr-use=") )
> 
> I think it would be easier for now to have only %clang substitution and use verbatim flags in lit tests
> (%clang -fprofile-instr-use=%t ...). 
> 

I left these -- once this initial commit is in, I (obviously) plan to add
more coverage.  Most tests will need both a generate and a use line.  I
think the substitutions will make the RUN lines more readable and easier
to maintain.

Or is there another reason to remove them?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: instrprof-tests-2.patch
Type: application/octet-stream
Size: 5496 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140328/6f242b4a/attachment.obj>