[PATCH] Code coverage mapping generation that enables coverage using the instrumentation based profiling
Alex L
arphaman at gmail.com
Mon Aug 4 11:51:30 PDT 2014
Commited in r214752.
2014-08-04 10:56 GMT-07:00 Alex L <arphaman at gmail.com>:
> Thanks, I'll fix #1 and remove the tests and will commit it then.
>
>
> 2014-08-04 10:53 GMT-07:00 Bob Wilson <bob.wilson at apple.com>:
>
> 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 <arphaman at gmail.com> wrote:
>>
>> Updated patch that uses the new coverage namespace.
>>
>>
>> 2014-07-21 10:09 GMT-07:00 Alex L <arphaman at gmail.com>:
>>
>>> Updated patch with renamed coverage mapping variable and
>>> -fcoverage-mapping option.
>>>
>>>
>>>
>>>
>>> 2014-07-18 17:40 GMT-07:00 Bob Wilson <bob.wilson at apple.com>:
>>>
>>>
>>>> On Jul 18, 2014, at 5:36 PM, Alex L <arphaman at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>>
>>>> 2014-07-18 17:34 GMT-07:00 Bob Wilson <bob.wilson at apple.com>:
>>>>
>>>>>
>>>>> On Jul 18, 2014, at 2:36 PM, Alex L <arphaman at gmail.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> 2014-07-18 14:23 GMT-07:00 Argyrios Kyrtzidis <kyrtzidis at apple.com>:
>>>>>
>>>>>> 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 <arphaman at gmail.com> 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 <kyrtzidis at apple.com>:
>>>>>>
>>>>>>> That sounds good to me.
>>>>>>>
>>>>>>> On Jul 18, 2014, at 1:38 PM, Alex L <arphaman at gmail.com> 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 <arphaman at gmail.com>:
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 2014-07-18 11:47 GMT-07:00 Argyrios Kyrtzidis <kyrtzidis at apple.com>
>>>>>>>> :
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Jul 18, 2014, at 9:56 AM, Sean Silva <chisophugis at gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, Jul 17, 2014 at 2:14 PM, Bob Wilson <bob.wilson at apple.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> > On Jul 8, 2014, at 10:43 AM, Alex L <arphaman at gmail.com> 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
>>>>>>>>>> cfe-commits at cs.uiuc.edu
>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> cfe-commits mailing list
>>>>>>>>> cfe-commits at cs.uiuc.edu
>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> <clangCoverageMapping.patch>
>>>>>>
>>>>>>
>>>>>>
>>>>> <clangCoverageMapping.patch>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>> <clangCoverageMapping.patch>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/1adb52d2/attachment.html>
More information about the cfe-commits
mailing list