[PATCH] Code coverage mapping generation that enables coverage using the instrumentation based profiling
Bob Wilson
bob.wilson at apple.com
Mon Aug 4 10:53:38 PDT 2014
Two comments:
1) For all the places where you are adding a new “CoverageInfo” argument to an existing function, please put that at the end of the argument list and provide a default nullptr value (at least where that makes sense). This will help to avoid breaking out-of-tree code using those functions.
2) I’d prefer to have test cases that directly test the mapping info emitted by clang. We can do that after the llvm-cov changes are committed. Let’s go ahead and commit this without the tests (but after you fix #1 above), and then we can add the tests in a follow-up commit.
> On Jul 21, 2014, at 10:59 AM, Alex L <arphaman at gmail.com> wrote:
>
> 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>
>>
>>
>
>
>
> <clangCoverageMapping.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/4f8b8720/attachment.html>
More information about the cfe-commits
mailing list