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
