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

Fri Jul 18 14:13:27 PDT 2014

This is the updated patch with the applied fixes, addition of
CoverageSourceInfo and the usage of one section for all of the coverage
mapping down.
Alex

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

> That sounds good to me.
>
> On Jul 18, 2014, at 1:38 PM, Alex L <arphaman at gmail.com> wrote:
>
> The 'CoverageSourceInfo' class that stores the skipped ranges is not a
> good approach because when the llvm codegenerator is created the
> preprocessing record doesn't actually have the skipped ranges as they
> aren't reached by the lexer yet.
> What about this: the 'CoverageSourceInfo' derives from PPCallbacks and
> stores it's own source ranges instead of relying on the preprocessing
> record. I think that this might be an even better approach as
> lib/Frontend/CompilerInstance.cpp wouldn't have to be modified.
> Alex
>
>
> 2014-07-18 12:06 GMT-07:00 Alex L <arphaman at gmail.com>:
>
>>
>>
>>
>> 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/9ee6bfac/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clangCoverageMapping.patch
Type: application/octet-stream
Size: 125992 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140718/9ee6bfac/attachment.obj>