> On Jul 18, 2014, at 5:36 PM, Alex L <[email protected]> wrote: > > > > > 2014-07-18 17:34 GMT-07:00 Bob Wilson <[email protected]>: > >> 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 > > 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 <[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
