[PATCH] Code coverage mapping generation that enables coverage using the instrumentation based profiling
Alex L
arphaman at gmail.com
Mon Jul 21 10:59:02 PDT 2014
Updated patch that uses the new coverage namespace.
2014-07-21 10:09 GMT-07:00 Alex L <arphaman at gmail.com>:
> Updated patch with renamed coverage mapping variable and
> -fcoverage-mapping option.
>
>
>
>
> 2014-07-18 17:40 GMT-07:00 Bob Wilson <bob.wilson at apple.com>:
>
>
>> On Jul 18, 2014, at 5:36 PM, Alex L <arphaman at gmail.com> wrote:
>>
>>
>>
>>
>> 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'?
>>
>>
>> Here I am asking for more verbose variable name (see above), but that
>> seems unnecessarily long to me. How about shortening it to
>> “-fcoverage-mapping”?
>>
>> You should also add a check to make sure it is only used with
>> -fprofile-instr-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/20140721/6463095a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clangCoverageMapping.patch
Type: application/octet-stream
Size: 125867 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140721/6463095a/attachment.obj>
More information about the cfe-commits
mailing list