[PATCH] Code coverage mapping generation that enables coverage using the instrumentation based profiling
Alex L
arphaman at gmail.com
Fri Jul 18 14:36:49 PDT 2014
2014-07-18 14:23 GMT-07:00 Argyrios Kyrtzidis <kyrtzidis at apple.com>:
> CodeGenABITypes::CodeGenABITypes(ASTContext &C,
> llvm::Module &M,
> - const llvm::DataLayout &TD)
> + const llvm::DataLayout &TD,
> + CoverageSourceInfo &CoverageInfo)
>
> Shouldn’t this be optional pointer ("CoverageSourceInfo *CoverageInfo =
> nullptr”) that will receive an object only when ‘ProfileInstrGenerate’
> option is enabled ?
>
Yes, this is a good idea. I've updated the patch.
>
> On Jul 18, 2014, at 2:13 PM, Alex L <arphaman at gmail.com> wrote:
>
> 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
>>>>
>>>>
>>>>
>>>
>>
>>
> <clangCoverageMapping.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140718/b87f62e9/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clangCoverageMapping.patch
Type: application/octet-stream
Size: 126043 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140718/b87f62e9/attachment.obj>
More information about the cfe-commits
mailing list