[PATCH] Code coverage mapping generation that enables coverage using the instrumentation based profiling
Alex L
arphaman at gmail.com
Fri Jul 18 17:36:02 PDT 2014
2014-07-18 17:34 GMT-07:00 Bob Wilson <bob.wilson at apple.com>:
>
> On Jul 18, 2014, at 2:36 PM, Alex L <arphaman at gmail.com> wrote:
>
>
>
>
> 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.
>
>
> Two new comments:
>
> 1) Can you rename the variable used to hold the coverage mapping from
> “__llvm_covmapping” to “__llvm_coverage_mapping”?
>
> 2) I think it would be good to put this under control of an “experimental”
> command line option while we refine the details. We don’t want to break
> anyone using -fprofile-instr-generate, and leaving it disabled by default
> would also make it clear that this is still a work in progress
>
What about '-fcoverage-mapping-generate'?
>
>
>>
>> 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>
>>
>>
>>
> <clangCoverageMapping.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140718/657143c4/attachment.html>
More information about the cfe-commits
mailing list