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

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?
2 - Any feedback on this specific check?
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?

Thanks,
Breno Rodrigues Guimarães

Hello!

I agree it’s controversial. I would say that some people would like that and others don’t. In my humble opinion it should be ok to add this check to clang-tidy.

Possibly to avoid some spurious warnings you could warn for const pointers only. I personally think that parameters that are written should be passed by pointer “getvalue(&x)” is more explicit imho than “getvalue(x)”.

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?

I don’t know if there is an official list anywhere.

But I would assume there are many ideas.

Personally I would like that more types of UB is detected. Here is some testcode with various UB, Clang detects some of the UB but not all:

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

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAAt6xTtzDzbTmKwi_RjTuJt70mZ7RHL1GbR_2Bi5EUJTMSFBiA@mail.gmail.com>,
    Aaron Ballman via cfe-dev <cfe-dev@lists.llvm.org> writes:

> 2 - Any feedback on this specific check?

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.

One very common case from programmers with a C background is to check
the result of new against NULL/nullptr/0. Other static analyzers like
cppcheck have such a check already. If clang/clang-tidy doesn't
already warn on code like this, that would be a good addition.

Another check in the same spirit is where code checks for non-nullptr
at some point and then checks against nullptr again. This usually
happens in large functions where the earlier check against nullptr is
"out of sight, out of mind" when someone is adding the second check
against nullptr.

Again, this would be a welcome addition to clang/clang-tidy if we
don't already have something like that.

Hi guys,

Thanks for all the suggestions! I’m starting to run my check in our code base (around 2M C++ lines) and the ratio of good findings and noise.

But as many of you said, there are already several well agreed guidelines that can be checked, and I will start picking some of them and produce the checks. Hopefully I can contribute very soon (I can only play with this at nights and weekends, but still…).

I also liked the idea of adding configurations and tweaks to existing checks. That’s a good way to see how the APIs are used and what’s available.

I appreciate the responses!

Best regards,
Breno G.