That sounds good to me. > On Jul 18, 2014, at 1:38 PM, Alex L <[email protected]> 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 <[email protected]>: > > > > 2014-07-18 11:47 GMT-07:00 Argyrios Kyrtzidis <[email protected]>: > > >> 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 ? > > > 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 >> [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
