<div dir="ltr"><div class="gmail_extra">LGTM for this patch, with a small nit:</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">+# Tool-specific config options.</div><div class="gmail_extra">
+config.profile_lit_source_dir = "@PROFILE_LIT_SOURCE_DIR@"</div><div>+config.host_triple = "@LLVM_HOST_TRIPLE@"<br></div></div><div class="gmail_extra"><div>^^^</div><div>You don't use config.host_triple or config.profile_lit_source_dir anywhere. Probably they can be removed.</div>
<div><br></div><br><div class="gmail_quote">On Fri, Mar 28, 2014 at 11:27 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Thanks for your review -- this was really helpful.  New patch attached<br>

that addresses most of your comments (exceptions below).<br>
<div class=""><br>
On 2014 Mar 28, at 09:02, Alexey Samsonov <<a href="mailto:samsonov@google.com">samsonov@google.com</a>> wrote:<br>
<br>
> Hi Duncan,<br>
><br>
> On Fri, Mar 28, 2014 at 8:21 AM, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
> +samsonov<br>
><br>
><br>
> On Fri, Mar 28, 2014 at 3:39 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> This patch adds the infrastructure for compiler-rt tests for instrumented profiling.<br>
> It also adds a single very simple test.<br>
><br>
> I’m a little lost with lit and cmake; could someone who knows better have a look to<br>
> make sure I have the right magic?<br>
><br>
> I copied and modified the logic from the test/asan/ directory.<br>
><br>
> There are two things that don’t (or probably don’t) work.<br>
><br>
>  1. You cannot call:<br>
><br>
>         $ cd llvm/projects/compiler-rt<br>
>         $ path/to/llvm-lit test/profile<br>
><br>
>     The other compiler-rt tests seem to have the same defect, and I’m not sure how<br>
>     to fix it.<br>
><br>
>     Nevertheless, using cmake+ninja, you can call:<br>
><br>
>         $ ninja check-profile<br>
><br>
>  2. I didn’t do anything with configure+make, since the other tests in compiler-rt<br>
>     didn’t have a model I could copy.  So the tests probably aren’t registered<br>
>     there.<br>
><br>
> Thanks for any pointers!<br>
><br>
> Duncan<br>
><br>
><br>
> Could you please use phabricator (<a href="http://llvm.org/docs/Phabricator.html" target="_blank">http://llvm.org/docs/Phabricator.html</a>) for sending patches?<br>
<br>
</div>I'm continuing this review in the same thread to keep things simple.  I'll<br>
try to remember your preference next time!<br>
<div class=""><br>
><br>
> A few comments:<br>
><br>
> +<br>
> +# Tests need to run generated executables.<br>
> +if config.host_triple != config.target_triple:<br>
> +  config.unsupported = True<br>
><br>
> This is not needed - if tests target different platform, COMPILER_RT_CAN_EXECUTE_TESTS should be OFF,<br>
> and the test suite wouldn't be added.<br>
><br>
><br>
> +set(PROFILE_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})<br>
> +<br>
> +set(PROFILE_TESTSUITES)<br>
><br>
> ^^ Looks like this is not used. Please delete.<br>
><br>
> +<br>
> +set(PROFILE_TEST_DEPS)<br>
> +if(NOT COMPILER_RT_STANDALONE_BUILD)<br>
> +  list(APPEND PROFILE_TEST_DEPS profile)<br>
><br>
> You also need to add dependency on clang, and likely clang-headers and llvm-profdata.<br>
><br>
> +endif()<br>
><br>
><br>
> +<br>
> +# Add clang substitutions.<br>
> +config.substitutions.append( ("%clang ", config.clang + " ") )<br>
> +config.substitutions.append( ("%clang_profgen ", config.clang + " -fprofile-instr-generate ") )<br>
> +config.substitutions.append( ("%clang_profuse=", config.clang + " -fprofile-instr-use=") )<br>
><br>
> I think it would be easier for now to have only %clang substitution and use verbatim flags in lit tests<br>
> (%clang -fprofile-instr-use=%t ...).<br>
><br>
<br>
</div>I left these -- once this initial commit is in, I (obviously) plan to add<br>
more coverage.  Most tests will need both a generate and a use line.  I<br>
think the substitutions will make the RUN lines more readable and easier<br>
to maintain.<br>
<br>
Or is there another reason to remove them?<br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div>
</div></div>