> On Jul 18, 2014, at 9:56 AM, Sean Silva <[email protected]> wrote:
>
>
>
>
> On Thu, Jul 17, 2014 at 2:14 PM, Bob Wilson <[email protected]> wrote:
>
> > On Jul 8, 2014, at 10:43 AM, Alex L <[email protected]> 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 ?
>
> 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
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
