> On Jul 18, 2014, at 2:36 PM, Alex L <[email protected]> wrote: > > > > > 2014-07-18 14:23 GMT-07:00 Argyrios Kyrtzidis <[email protected]>: > 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. > > >> On Jul 18, 2014, at 2:13 PM, Alex L <[email protected]> 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 <[email protected]>: >> 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 >>> >>> >>> >> >> >> <clangCoverageMapping.patch> > > > <clangCoverageMapping.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
