[PATCH] Code coverage mapping generation that enables coverage using the instrumentation based profiling
Argyrios Kyrtzidis
kyrtzidis at apple.com
Fri Jul 18 13:43:24 PDT 2014
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/49488386/attachment.html>
More information about the cfe-commits
mailing list