> 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.
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
