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 ?
> 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>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
