<div dir="ltr">Updated patch with renamed coverage mapping variable and -fcoverage-mapping option.<br><br><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-18 17:40 GMT-07:00 Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class=""><blockquote type="cite"><div>On Jul 18, 2014, at 5:36 PM, Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:</div>
<br><div><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-18 17:34 GMT-07:00 Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div><blockquote type="cite"><div>On Jul 18, 2014, at 2:36 PM, Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:</div>
<br><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div class="gmail_extra"><br><br><br><div class="gmail_quote">2014-07-18 14:23 GMT-07:00 Argyrios Kyrtzidis<span> </span><span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div> CodeGenABITypes::CodeGenABITypes(ASTContext &C,</div>
<div> llvm::Module &M,</div><div>- const llvm::DataLayout &TD)</div><div>+ const llvm::DataLayout &TD,</div><div>
+ CoverageSourceInfo &CoverageInfo)</div></div><div><br></div><div>Shouldn’t this be optional pointer ("CoverageSourceInfo *CoverageInfo = nullptr”) that will receive an object only when ‘ProfileInstrGenerate’ option is enabled ?</div>
</div></blockquote><div><br></div><div>Yes, this is a good idea. I've updated the patch.<br></div></div></div></div></div></blockquote><div><br></div></div>Two new comments:</div><div><br></div><div>1) Can you rename the variable used to hold the coverage mapping from “__llvm_covmapping” to “__llvm_coverage_mapping”?</div>
<div><br></div><div>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<br>
</div></div></div></blockquote><div><br></div><div>What about '-fcoverage-mapping-generate'? <br></div></div></div></div></div></blockquote><div><br></div></div>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”?</div>
<div><br></div><div>You should also add a check to make sure it is only used with -fprofile-instr-generate.</div><div><div class="h5"><div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div><blockquote type="cite"><div><div><div dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><blockquote type="cite"><div><div><div>On Jul 18, 2014, at 2:13 PM, Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:</div><br></div>
</div><div><div><div dir="ltr"><div>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.<br></div>Alex<br></div><div class="gmail_extra">
<br><br><div class="gmail_quote">2014-07-18 13:43 GMT-07:00 Argyrios Kyrtzidis<span> </span><span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>That sounds good to me.</div><div><br><div><blockquote type="cite"><div>On Jul 18, 2014, at 1:38 PM, Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>> wrote:</div>
<br><div><div dir="ltr">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.<br>
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.<br>
Alex<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-07-18 12:06 GMT-07:00 Alex L<span> </span><span dir="ltr"><<a href="mailto:arphaman@gmail.com" target="_blank">arphaman@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
2014-07-18 11:47 GMT-07:00 Argyrios Kyrtzidis<span> </span><span dir="ltr"><<a href="mailto:kyrtzidis@apple.com" target="_blank">kyrtzidis@apple.com</a>></span>:<div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br></div><div><div><blockquote type="cite"><div>On Jul 18, 2014, at 9:56 AM, Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>> wrote:</div>
<br><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> </span><span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div><br>> On Jul 8, 2014, at 10:43 AM, Alex L <<a href="mailto:arphaman@gmail.com" target="_blank">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>> <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><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></div></div></blockquote><div><br></div></div><div>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.<br>
</div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div></div>
<blockquote type="cite"><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-width:1px;border-left-style:solid;border-left-color: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" target="_blank">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" target="_blank">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></blockquote></div></div><br></div></div></blockquote></div><br></div></div></blockquote></div><br></div></div></blockquote></div><br></div></div><span><clangCoverageMapping.patch></span></div>
</blockquote></div><br></div></blockquote></div><br></div></div></div></div><span><clangCoverageMapping.patch></span></blockquote></div><br></div></div></blockquote></div><br></div></div>
</div></blockquote></div><br></div></div></div></blockquote></div><br></div>