Two comments: 1) For all the places where you are adding a new “CoverageInfo” argument to an existing function, please put that at the end of the argument list and provide a default nullptr value (at least where that makes sense). This will help to avoid breaking out-of-tree code using those functions.
2) I’d prefer to have test cases that directly test the mapping info emitted by clang. We can do that after the llvm-cov changes are committed. Let’s go ahead and commit this without the tests (but after you fix #1 above), and then we can add the tests in a follow-up commit. > On Jul 21, 2014, at 10:59 AM, Alex L <[email protected]> wrote: > > Updated patch that uses the new coverage namespace. > > > 2014-07-21 10:09 GMT-07:00 Alex L <[email protected]>: > Updated patch with renamed coverage mapping variable and -fcoverage-mapping > option. > > > > > 2014-07-18 17:40 GMT-07:00 Bob Wilson <[email protected]>: > > >> 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> >> >> > > > > <clangCoverageMapping.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
