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

Ted Kremenek kremenek at apple.com
Sat Feb 1 23:00:22 PST 2014 

On Jan 29, 2014, at 7:45 AM, Alexander Kornienko <alexfh at google.com> wrote:

> On Sat, Jan 25, 2014 at 7:50 AM, Ted Kremenek <kremenek at apple.com> wrote:
> 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've renamed CheckerNameWrapper to CheckerName. As for the need in this class in general, it serves as a guard against random StringRefs being passed to BugType. This was suggested by Jordan (I'm not sure, that the implementation is exactly what he imagined, though), and I think it's a reasonable precaution.

Yes, it sounds good to me.  ‘CheckerName’ is a big improvement over ‘CheckerNameWrapper’, and we can always refine the terminology later.

>  
> ...
> 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.
> 
> I was thinking about this alternative before, but chose the other one to avoid dealing with a mutable shared state. If you prefer this option, I'm happy to change it.

It’s tradeoffs, but I don’t see it as a big deal.  CheckerManager isn’t thread-safe when it is being mutated in any way, and all this work happens during initialization.  I’d rather do this then punish checker writers with a bunch of redundant boilerplate.

Jordan: what are your opinions on this approach?

>  
> Aside from that, is there a reason we want to traffic at all with CheckerNameWrapper (or rather, CheckerName) for calls to things like EmitBasicReport?
> 
> I've added an overload of the EmitBasicReport, which receives a pointer to CheckerBase instead of the checker name. There's one corner-case though: checkers, that implement several sets of checks (MallocChecker, CStringChecker and a few others). These checkers need to pass the exact checker name, which can't be replaced with a reference to the checker.

Ah, yes.  This concept of “checks” and “checkers” aren’t totally formalized (with the latter implementing the former).  What “CheckerName” corresponds to is the specific “Check” or “Checker Diagnostic”.  I think this is fine for now, but I think this concept will need to be refined over time.

>  
>  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?
> 
> It would be interesting , but, as I've said, we can't just replace the checker name with a reference to the checker.

Yes, I forgot about this obviously important point.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140201/6c7653e6/attachment.html>

More information about the cfe-commits mailing list