[PATCH] InstrProf: Add initial compiler-rt test

[Alexey Samsonov]

LGTM for this patch, with a small nit:

+# Tool-specific config options.
+config.profile_lit_source_dir = "@PROFILE_LIT_SOURCE_DIR@"
+config.host_triple = "@LLVM_HOST_TRIPLE@"
^^^
You don't use config.host_triple or config.profile_lit_source_dir anywhere.
Probably they can be removed.


On Fri, Mar 28, 2014 at 11:27 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> 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?
>
>


-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140331/787800f5/attachment.html>