[PATCH] Code coverage mapping generation that enables coverage using the instrumentation based profiling

[Alex L]

2014-07-18 11:47 GMT-07:00 Argyrios Kyrtzidis <kyrtzidis at apple.com>:

>
> On Jul 18, 2014, at 9:56 AM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
>
>
> On Thu, Jul 17, 2014 at 2:14 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
>>
>> > On Jul 8, 2014, at 10:43 AM, Alex L <arphaman at gmail.com> wrote:
>> >
>> > Hi everyone,
>> >
>> > I've attached a patch with the initial implementation of the code
>> coverage mapping generation that
>> > enables code coverage analysis which uses the data obtained from the
>> instrumentation based profiling.
>> >
>> > I've sent out the patches for the coverage mapping format library and
>> the updated coverage tool in separate threads.
>> >
>> > Cheers,
>> > Alex
>> > <clangCoverageMapping.patch>
>>
>> This looks really nice! It is obviously blocked by getting the llvm
>> changes in, but it is otherwise mostly ready to commit. I just have a few
>> small comments.
>>
>> It is unfortunate that you have to propagate the Preprocessor through a
>> bunch of code to make it available in CodeGen. I can’t think of any good
>> alternative, though. It would be good to get someone more familiar with the
>> overall structure of the front-end to review that part.
>>
>
> Agreed. That seems sort of funky. Does the code use anything other than
> the PreprocessingRecord? Could we just pass that down instead of the full
> Preprocessor?
>
>
> Looks like only SkippedRanges are used from the PreprocessingRecord. Could
> we have something like ‘CoverageSourceInfo’ class containing the
> SkippedRanges (and anything else useful) and thread this through to CodeGen
> ?
>
>
Yes, only the SkippedRanges are used. A separate class like
'CoverageSourceInfo' sounds like a good idea, I will pass it instead of the
preprocesor to CodeGen.

>
> Notwithstanding my suggestion, someone with better knowledge of the
> layering here should sign off before this gets committed.
>
> -- Sean Silva
>
>
>>
>> I also noticed that you are adding a number of functions that don’t
>> follow the naming convention of starting with a lowercase letter. I know
>> there is a lot of code in clang that doesn’t follow that convention, and
>> perhaps you are doing it that way on purpose to be consistent, but please
>> review all the new function names and follow the coding standard, except
>> for any cases where it clearly makes more sense to match the existing code.
>>
>> > @@ -807,6 +848,17 @@ static void emitRuntimeHook(CodeGenModule &CGM) {
>> >    CGM.addUsedGlobal(User);
>> >  }
>> >
>> > +void CodeGenPGO::checkGlobalDecl(GlobalDecl GD) {
>> > +  // Make sure we only emit coverage mapping for one
>> > +  // constructor/destructor
>>
>> Please elaborate on this comment to explain why it is an issue.
>>
>> > +  if ((isa<CXXConstructorDecl>(GD.getDecl()) &&
>> > +       GD.getCtorType() != Ctor_Base) ||
>> > +      (isa<CXXDestructorDecl>(GD.getDecl()) &&
>> > +       GD.getDtorType() != Dtor_Base)) {
>> > +    SkipCoverageMapping = true;
>> > +  }
>> > +}
>> > +
>> >  void CodeGenPGO::assignRegionCounters(const Decl *D, llvm::Function
>> *Fn) {
>> >    bool InstrumentRegions = CGM.getCodeGenOpts().ProfileInstrGenerate;
>> >    llvm::IndexedInstrProfReader *PGOReader = CGM.getPGOReader();
>>
>> …
>>
>> > diff --git a/lib/CodeGen/CoverageMappingGen.cpp
>> b/lib/CodeGen/CoverageMappingGen.cpp
>> > new file mode 100644
>> > index 0000000..ed65660
>> > --- /dev/null
>> > +++ b/lib/CodeGen/CoverageMappingGen.cpp
>> > @@ -0,0 +1,1178 @@
>> > +//===--- CoverageMappingGen.cpp - Coverage mapping generation ---*-
>> C++ -*-===//
>> > +//
>> > +//                     The LLVM Compiler Infrastructure
>> > +//
>> > +// This file is distributed under the University of Illinois Open
>> Source
>> > +// License. See LICENSE.TXT for details.
>> > +//
>> >
>> +//===----------------------------------------------------------------------===//
>> > +//
>> > +// Instrumentation-based code coverage mapping generator
>> > +//
>> >
>> +//===----------------------------------------------------------------------===//
>> > +
>> > +#include "CoverageMappingGen.h"
>> > +#include "CodeGenFunction.h"
>> > +#include "clang/AST/RecursiveASTVisitor.h”
>>
>> I don’t see any direct use of RecursiveASTVisitor in this file. Is this
>> #include really needed?
>>
>> …
>>
>> > +/// \brief A StmtVisitor that creates unreachable coverage regions for
>> the
>> > +/// functions that are not emitted.
>> > +struct EmptyCoverageMappingBuilder : public CoverageMappingBuilder {
>>
>> The comment is wrong — this is not actually a StmtVisitor.
>>
>> …
>>
>> > +/// \brief A StmtVisitor that creates coverage mapping regions maps the
>> > +/// source code locations to PGO counters.
>> > +struct CounterCoverageMappingBuilder
>> > +    : public CoverageMappingBuilder,
>> > +      public ConstStmtVisitor<CounterCoverageMappingBuilder> {
>>
>> The comment here isn’t a proper sentence. Maybe you intended “maps” to be
>> “that map”?
>>
>> The rest of this patch looks really good to me.
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140718/0793df7a/attachment.html>