Commited in r214752.
2014-08-04 10:56 GMT-07:00 Alex L <[email protected]>: > Thanks, I'll fix #1 and remove the tests and will commit it then. > > > 2014-08-04 10:53 GMT-07:00 Bob Wilson <[email protected]>: > > 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
