[cfe-dev] Why these problems below were not found by Clang Static Analyzer?

[Richard Smith]

On Fri, Mar 14, 2014 at 10:00 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> The analyzer tries not to give results that are already covered by regular
> Clang warnings. This is sometimes problematic, since the Clang warnings may
> not be on by default. Nevertheless...
>
> On Mar 11, 2014, at 19:53 , apache <ehcapa at qq.com> wrote:
>
> ----------------------------------------------------------------------
> 1.case without break
> e.g.
> int test(const int n) {
>     int ret = 0;
>     switch(n) {
>     case 1:
>         ret = 1;
>         break;
>     case 2:
>         ret = 2; // this case branch has no 'break' statement.(coverity
> gived a warning here, but Clang didn't)
>     default:
>         break;
>     }
>     return ret;
> }
>
>
> This is supposed to be -Wimplicit-fallthrough, but I don't see it working
> either! Richard, do you know what's going on here?
>

Yes. We originally wanted to warn here, but:

 1) some people are suggesting to build with -Weverything -Werror,
 2) it was thought that there was no nice way to suppress this on a
case-by-case basis outside C++11 mode, and
 3) there were concerns that breaking non-C++11 -Weverything -Werror builds
was a bad idea
so as a compromise the warning ended up disabled if you're not in C++11
mode.

The end result here is pretty unfortunate. I think we can do better. Here's
a macro that should suppress the warning:

#define FALLTHROUGH2(LINE) goto _fallthrough_##LINE; _fallthrough_##LINE:
#define FALLTHROUGH1(LINE) FALLTHROUGH2(LINE)
#define FALLTHROUGH FALLTHROUGH1(__LINE__)

This isn't *quite* as good as "[[clang::fallthrough]];", because we would
allow it outside switch statements (and in cases where it doesn't precede a
case label), and because you can only put one such macro on each line, but
it's a pretty good simulacrum. (Also, it doesn't suppress the warning if
the use of the macro is followed by a semicolon, which seems like a bug in
the warning.)

I think we should add the above macro to the documentation and remove the
CPlusPlus11 check from the diagnostic. Thoughts?

> ----------------------------------------------------------------------
> 2.Dead code like below
> #define MAX_NUM 10
> void test(const int n) {
>     if(n >= MAX_NUM && n < MAX_NUM) {
>         printf("yes\n"); // this code will never be executed!(coverity
> gived a warning here, but Clang didn't)
>     }
> }
>
>
> This is a true deficiency in the analyzer and in -Wunreachable-code: the
> former isn't yet good at answering questions that are true "for all paths
> through the code" (dead stores being an exception), and the latter isn't
> smart enough to handle the combination of constraints on 'n'.
>
>
> ----------------------------------------------------------------------
> 3.NULL-Pointer reference like below
> typedef struct {
>     int age;
>     int sex;
> } Person;
>
> Person *one_person(char flag)
> {
>     static Person p = {0, 0};
>     if(flag == 1) {
>         return &p;
>     }
>     return 0;
> }
>
> void test()
> {
>     Person *p = on_person(0);
>     p->age = 24; // NULL-Pointer reference(coverity gived a warning here,
> but Clang didn't)
>     p->sex = 0;
> }
>
>
> This is a heuristic in the analyzer, assuming that in most cases you won't
> actually be taking the null-return case of a function you call. This is
> important for things like std::map::operator[], which will create a new
> element if the key isn't already in the map. If the value type is a pointer
> type, this is going to give back a null pointer and cause a spurious
> analyzer report.
>
> I've had a bug on me for a while to make this work if we can *prove* that
> the null pointer return isn't the result of incomplete knowledge (i.e. that
> the analyzer didn't have to guess at the value of any of the parameters),
> but I haven't gotten around to it yet. It's something we have to do
> carefully: the analyzer has to strike a balance between false negatives and
> false positives, and that balance is different from Coverity's for a
> variety of reasons.
>
> Thanks for the reports. If you're interested in seeing them followed up on
> specifically, please file bugs at http://llvm.org/bugs/
>
> Jordan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140314/bb9fbc28/attachment.html>