<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div><blockquote type="cite"><div>On Jul 18, 2014, at 9:56 AM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 17, 2014 at 2:14 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5"><br>
> On Jul 8, 2014, at 10:43 AM, Alex L <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
><br>
> Hi everyone,<br>
><br>
> I've attached a patch with the initial implementation of the code coverage mapping generation that<br>
> enables code coverage analysis which uses the data obtained from the instrumentation based profiling.<br>
><br>
> I've sent out the patches for the coverage mapping format library and the updated coverage tool in separate threads.<br>
><br>
> Cheers,<br>
> Alex<br>
</div></div>> <clangCoverageMapping.patch><br>
<br>
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.<br>
<br>
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.<br>
</blockquote><div><br></div><div>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?<br></div></div></div></div></div></blockquote><div><br></div><div>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 ?</div><div><br></div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>Notwithstanding my suggestion, someone with better knowledge of the layering here should sign off before this gets committed.<br>
</div><div><br></div><div>-- Sean Silva<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
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.<br>
<br>
> @@ -807,6 +848,17 @@ static void emitRuntimeHook(CodeGenModule &CGM) {<br>
> CGM.addUsedGlobal(User);<br>
> }<br>
><br>
> +void CodeGenPGO::checkGlobalDecl(GlobalDecl GD) {<br>
> + // Make sure we only emit coverage mapping for one<br>
> + // constructor/destructor<br>
<br>
Please elaborate on this comment to explain why it is an issue.<br>
<br>
> + if ((isa<CXXConstructorDecl>(GD.getDecl()) &&<br>
> + GD.getCtorType() != Ctor_Base) ||<br>
> + (isa<CXXDestructorDecl>(GD.getDecl()) &&<br>
> + GD.getDtorType() != Dtor_Base)) {<br>
> + SkipCoverageMapping = true;<br>
> + }<br>
> +}<br>
> +<br>
> void CodeGenPGO::assignRegionCounters(const Decl *D, llvm::Function *Fn) {<br>
> bool InstrumentRegions = CGM.getCodeGenOpts().ProfileInstrGenerate;<br>
> llvm::IndexedInstrProfReader *PGOReader = CGM.getPGOReader();<br>
<br>
…<br>
<br>
> diff --git a/lib/CodeGen/CoverageMappingGen.cpp b/lib/CodeGen/CoverageMappingGen.cpp<br>
> new file mode 100644<br>
> index 0000000..ed65660<br>
> --- /dev/null<br>
> +++ b/lib/CodeGen/CoverageMappingGen.cpp<br>
> @@ -0,0 +1,1178 @@<br>
> +//===--- CoverageMappingGen.cpp - Coverage mapping generation ---*- C++ -*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +//<br>
> +// Instrumentation-based code coverage mapping generator<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#include "CoverageMappingGen.h"<br>
> +#include "CodeGenFunction.h"<br>
> +#include "clang/AST/RecursiveASTVisitor.h”<br>
<br>
I don’t see any direct use of RecursiveASTVisitor in this file. Is this #include really needed?<br>
<br>
…<br>
<br>
> +/// \brief A StmtVisitor that creates unreachable coverage regions for the<br>
> +/// functions that are not emitted.<br>
> +struct EmptyCoverageMappingBuilder : public CoverageMappingBuilder {<br>
<br>
The comment is wrong — this is not actually a StmtVisitor.<br>
<br>
…<br>
<br>
> +/// \brief A StmtVisitor that creates coverage mapping regions maps the<br>
> +/// source code locations to PGO counters.<br>
> +struct CounterCoverageMappingBuilder<br>
> + : public CoverageMappingBuilder,<br>
> + public ConstStmtVisitor<CounterCoverageMappingBuilder> {<br>
<br>
The comment here isn’t a proper sentence. Maybe you intended “maps” to be “that map”?<br>
<br>
The rest of this patch looks really good to me.<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>
_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br></div></blockquote></div><br></body></html>