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

Alexander Kornienko alexfh at google.com
Wed Jan 29 07:45:24 PST 2014


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.


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


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


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


>
> 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/20140129/4ff3b837/attachment.html>


More information about the cfe-commits mailing list