[PATCH] Expose the name of the checker producing each diagnostic message.

[Ted Kremenek]

Hi Alex,

Thank you for working on this.  I’m going to review the patch in its entirety.  I’m glad to see this information get pushed through, but I’m a bit concerned about the approach.

A few high bits:

(1) This patch modifies every call to registerChecker<T>() just to pass through an extra string.  This seems like excessive boilerplate.

(2) The name ‘CheckerNameWrapper’ is a bit too precise to the point of being confusing.  The “Wrapper” part has no meaning unless you look at the implementation of the class.  If we want to have a custom type here, “CheckerName” seems to capture the intention more succinctly.  I’m also not certain if adding this extra type is necessary; a StringRef is probably fine.

I haven’t thought this through completely, but consider an alternate approach.  Instead of modifying all registerXXXChecker() functions which impacts ALL checker implementers, maybe we can just modify CheckerRegistery::initializeManager(), whose current implementation is this:

> void CheckerRegistry::initializeManager(CheckerManager &checkerMgr, 
>                                   SmallVectorImpl<CheckerOptInfo> &opts) const {
>   // Sort checkers for efficient collection.
>   std::sort(Checkers.begin(), Checkers.end(), checkerNameLT);
> 
>   // Collect checkers enabled by the options.
>   CheckerInfoSet enabledCheckers;
>   for (SmallVectorImpl<CheckerOptInfo>::iterator
>          i = opts.begin(), e = opts.end(); i != e; ++i) {
>     collectCheckers(Checkers, Packages, *i, enabledCheckers);
>   }
> 
>   // Initialize the CheckerManager with all enabled checkers.
>   for (CheckerInfoSet::iterator
>          i = enabledCheckers.begin(), e = enabledCheckers.end(); i != e; ++i) {
>     (*i)->Initialize(checkerMgr);
>   }

There’s a single loop that goes and creates all the checker objects, which (I believe) includes both builtin checkers and ones loaded from plugins.  The CheckerInfo object contains the full string of the checker.  What if we changed the loop body to something like this:

{
      checkerManager.setCheckerName((*I)->FullName;
      (*i)->Initialize(checkerMgr);
}

Then in CheckerManager, we can modify CheckerManager::registerChecker<T>() to consult that StringRef, and if it is there, use it to splat the name into the CheckerBase object.  You then don’t have to touch the individual registerXXXChecker() functions at all.  It’s less boilerplate for checker authors, and also less error prone.  It is also extensible if we ever want to plumb in more data.  I know it’s a bit gross since it’s meddling with some shared state in CheckerManager, but this initialization really only happens up front during startup.

Aside from that, is there a reason we want to traffic at all with CheckerNameWrapper (or rather, CheckerName) for calls to things like EmitBasicReport?  For example:

>  void BugReporter::EmitBasicReport(const Decl *DeclWithIssue,
> -                                  StringRef name,
> -                                  StringRef category,
> +                                  CheckerNameWrapper checkerName,
> +                                  StringRef name, StringRef category,
>                                    StringRef str, PathDiagnosticLocation Loc,
>                                    ArrayRef<SourceRange> Ranges) {
>  


Why not always just traffic with Checker&, and avoid introducing one more concept?  This seems a bit more general.  If we ever want to retrieve more information from the Checker, we have a reference to the actual Checker object, and not just a string.  The following line could then be written from…

>  llvm::raw_svector_ostream(fullDesc) << checkerName.getName() << ":" << name


to

>  llvm::raw_svector_ostream(fullDesc) << checker.getName() << ":" << category;


Not only does that seem cleaner, it’s one less concept for the BugReporter system to think about.  I’m even wondering if PathDiagnostic would benefit from just getting a reference to the entire Checker, for example:

> PathDiagnostic::PathDiagnostic(StringRef checkerName, const Decl *declWithIssue,


would become:

> PathDiagnostic::PathDiagnostic(Checker &checker, const Decl *declWithIssue,

That also seems more general.  As long as the PathDiagnostic object has a shorter lifetime than the Checker object than this seems perfectly safe to me.

What do you think?

Cheers,
Ted

On Jan 22, 2014, at 5:32 AM, Alexander Kornienko <alexfh at google.com> wrote:

>  Avoid copying strings when creating BugTypes or reporting errors.
>  Introduced a wrapper type for checker names to ensure we only get StringRefs
>  from the CheckerRegistry.
> 
>> Much better! :-) I wish we could cut down a bit on the copying of the name, but
>> to do that we'd have to rely on the CheckerRegistry always living longer than
>> the diagnostics. We could probably assume that for BugType, though. (If we did
>> that, we'd want to use a wrapper data type so that people didn't accidentally
>> pass random strings.)
> 
>  Done.
> 
>> At the very least, though, even the combined checkers can use StringRef instead
>> of std::string to track the names of the individual checks.
> 
>  Done.
> 
>> It might be nice to have a BugType constructor overload that takes a Checker and
>> just immediately calls getCheckerName(), just so 90% the clients could pass
>> "this", but that's getting fairly nitpicky.
> 
>  Done for BugType and all its descendants.
> 
>> I'd also like Ted to at least give a thumbs-up before this goes in.
> 
>  Yes, would be nice to hear from Ted on this.
> 
> Hi jordan_rose, krememek,
> 
> http://llvm-reviews.chandlerc.com/D2557
> 
> CHANGE SINCE LAST DIFF
>  http://llvm-reviews.chandlerc.com/D2557?vs=6560&id=6568#toc
> 
> Files:
>  examples/analyzer-plugin/MainCallChecker.cpp
>  include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
>  include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
>  include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
>  include/clang/StaticAnalyzer/Core/Checker.h
>  include/clang/StaticAnalyzer/Core/CheckerManager.h
>  include/clang/StaticAnalyzer/Core/CheckerRegistry.h
>  lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp
>  lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
>  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
>  lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
>  lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
>  lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
>  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
>  lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
>  lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
>  lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
>  lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp
>  lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
>  lib/StaticAnalyzer/Checkers/CheckObjCInstMethSignature.cpp
>  lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
>  lib/StaticAnalyzer/Checkers/CheckSizeofPointer.cpp
>  lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
>  lib/StaticAnalyzer/Checkers/ClangSACheckers.h
>  lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
>  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
>  lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
>  lib/StaticAnalyzer/Checkers/DirectIvarAssignment.cpp
>  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
>  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
>  lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
>  lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
>  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
>  lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
>  lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
>  lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
>  lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp
>  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
>  lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
>  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>  lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp
>  lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp
>  lib/StaticAnalyzer/Checkers/NSAutoreleasePoolChecker.cpp
>  lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
>  lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
>  lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
>  lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
>  lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
>  lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
>  lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp
>  lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
>  lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp
>  lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
>  lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
>  lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
>  lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
>  lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
>  lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
>  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
>  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
>  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
>  lib/StaticAnalyzer/Checkers/TaintTesterChecker.cpp
>  lib/StaticAnalyzer/Checkers/TraversalChecker.cpp
>  lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
>  lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
>  lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
>  lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp
>  lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
>  lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
>  lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
>  lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
>  lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
>  lib/StaticAnalyzer/Core/BugReporter.cpp
>  lib/StaticAnalyzer/Core/Checker.cpp
>  lib/StaticAnalyzer/Core/CheckerRegistry.cpp
>  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
> <D2557.4.patch>