[cfe-dev] [clang-tidy] Check for paranoic code (pointer checking)

[Aaron Ballman via cfe-dev]

On Thu, Mar 16, 2017 at 12:54 AM, Breno Guimarães via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
> Hi all,
>
> I started exploring clang-tidy to create checks for a large C++ code base we
> have at work, with many developers with C background doing C++.
>
> After doing the first tutorial on VirtualShadowingCheck, I wrote a check to
> detect paranoic code using pointers. By paranoic, I mean this:
>
> static void foo(T* p, ...) {
>     if (!p) return;
>     // do something
> }
>
> The method is doing the right thing to check the input, but the interface
> itself could've been written with a reference, thus delegating to the caller
> the responsability of doing the null check.
>
> This is not at all always possible, since this style can be valid for
> several reasons: pointers can refer diverse conceptual objects, e.g.
> arrays), the function could be virtual, thus forced to follow the same
> parameters, and so on.
>
> But still, controversial as it is, I ran this check on LLVM code base.
>
> llvm/tools/clang/lib/Sema/SemaLookup.cpp:3453 is one example. (the check
> finds over 15 violations, but I haven't analyzed them all)
>
> This function is one example that could be ignoring a potential mistake. The
> caller could expect a valid pointer in at least some of those calls. The
> convenience of writing:
>
> foo(something->getPointer(), ...);
>
> can stimulate the developer to not write checks/assertions for some cases
> that he/she expects the pointer to be valid.
>
> All that being said, I have some questions:
>
> 1 - Is the community receptive to such controversial checks? Or is
> clang-tidy only interested in low false positive/ non controversial checks?

Clang-tidy checks can have a higher false-positive rate than, say,
compiler warnings, but only up to a point. We tend to avoid checks
that are overly chatty where developers are apt to want to disable
them by default.

> 2 - Any feedback on this specific check?

Having a check that blindly suggests removing checks for pointer
validity seems like a low-value check. Those pointer checks may or may
not be valid -- if they are not valid, the check is useful, but if
they are valid, the check is now suggesting something *very* dangerous
to the user. This means for each diagnostic, the user has to do a lot
of work to figure out whether the diagnostic is a false-positive or
not.

Adding some intelligence to the check so that it suggests removing the
validity check, or suggests replacing the pointer with a reference,
when sensible to do so would make this a much better check IMO.
However, I'm not certain it's appropriate for a clang-tidy check due
to it being path sensitive in nature. It seems like a more natural fit
for a static analyzer check if going that route.

> 3 - Are there more check ideas waiting for someone to implement so I could
> pick up, learn more and actually contribute to the code base?

One good place to start is with the checks for specific coding
standards. We currently have a module for CERT secure coding
guidelines (C and C++) and one for the C++ Core Guidelines. We're also
in the process of starting to add checks for the High Integrity C++
coding standard. The nice thing about adding checks for existing
coding standards is that the checks have a reasonably good
specification for what needs to be checked, and a built-in
justification for why it needs to be checked.

~Aaron

>
>
> Thanks,
> Breno Rodrigues Guimarães
>
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>