Why these problems below were not found by Clang Static Analyzer?

Clang Frontend
#1

As below, these problems can be found be Coverity. But no BUGS were reported useing scan-build;(
Is the reason that I did not choose the right checkers?
Hers is my command line:

scan-build --use-analyzer=/usr/local/bin/clang -enable-checker llvm.Conventions -enable-checker alpha.core.BoolAssignment -enable-checker alpha.core.CastSize -enable-checke
r alpha.core.CastToStruct -enable-checker alpha.core.FixedAddr -enable-checker alpha.core.IdenticalExpr -enable-checker alpha.core.PointerArithm -enable-checker alpha.core.
PointerSub -enable-checker alpha.core.SizeofPtr -enable-checker alpha.cplusplus.NewDeleteLeaks -enable-checker alpha.cplusplus.VirtualCall -enable-checker alpha.deadcode.Id
empotentOperations -enable-checker alpha.deadcode.UnreachableCode -enable-checker alpha.security.ArrayBound -enable-checker alpha.security.ArrayBoundV2 -enable-checker alph
a.security.MallocOverflow -enable-checker alpha.security.ReturnPtrRange -enable-checker alpha.security.taint.TaintPropagation -enable-checker alpha.unix.Chroot -enable-chec
ker alpha.unix.MallocWithAnnotations -enable-checker alpha.unix.PthreadLock -enable-checker alpha.unix.SimpleStream -enable-checker alpha.unix.Stream -enable-checker alpha.
unix.cstring.BufferOverlap -enable-checker alpha.unix.cstring.NotNullTerminated -enable-checker alpha.unix.cstring.OutOfBounds -enable-checker security.FloatLoopCounter -en
able-checker security.insecureAPI.rand -enable-checker security.insecureAPI.strcpy clang -c test2.c

#2

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…

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?

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

#3

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

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

In general, -Wimplicit-fallthrough does warn on this, which I mostly
consider a bug: Falling through to a default: branch that contains nothing
but a break is a) safe b) relatively common, so I think this shouldn't
warn. (But it currently does, at least in blink.)

#4

I tried adding a “ret = 3” too, and it still didn’t warn. I’m not sure why.

Jordan

#5

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

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

#6

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

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

Could just "// FALLTHROUGH" or " // Fall through." etc (basically a case
insensitive match for a comment containing exactly \w*fall\
?through[\.:]?\w*) suppress the warning? That'd work in all languages, and
would work with lots of existing code too (e.g. gperf's output, etc).

(I looked into turning this warning on for chromium / blink at one point,
at having to declare a macro like this in 4 repos was one of the annoying
bits of it. If just adding a comment worked, that would've been much
better.)

#7

We may implement the solution you suggested. I can also take a look at implementing a compiler built-in-based way to silence the diagnostic. I guess, it’s possible to restrict its usage to within switch statements only, unlike the hack with the labels. What do you think?

#8

We may implement the solution you suggested. I can also take a look at
implementing a compiler built-in-based way to silence the diagnostic. I
guess, it's possible to restrict its usage to within switch statements
only, unlike the hack with the labels. What do you think?

The downside with a builtin is that any project that wants to use this
warning then needs to have a

#if __clang__ && __has_feature(__switch_fallthrough_macro_thinger)
#define FALLTHROUGH
__builtin_magic_which_annotates_fallthroughs_but_might_potentially_come_in_handy_in_other_contexts
#else
#define FALLTHROUGH
#endif

somewhere. If you have a central header that's included ~everywhere, that's
not a problem, but when your source is a collection of independent modules
not sharing much code, this gets much more annoying. (You could arguably
define this through a -D flag, but it's still less convenient that having
the comments that everybody already uses just work.)

#9

We may implement the solution you suggested. I can also take a look at
implementing a compiler built-in-based way to silence the diagnostic. I
guess, it's possible to restrict its usage to within switch statements
only, unlike the hack with the labels. What do you think?

The downside with a builtin is that any project that wants to use this
warning then needs to have a

#if __clang__ && __has_feature(__switch_fallthrough_macro_thinger)
#define FALLTHROUGH
__builtin_magic_which_annotates_fallthroughs_but_might_potentially_come_in_handy_in_other_contexts
#else
#define FALLTHROUGH
#endif

somewhere. If you have a central header that's included ~everywhere,
that's not a problem, but when your source is a collection of independent
modules not sharing much code, this gets much more annoying. (You could
arguably define this through a -D flag, but it's still less convenient that
having the comments that everybody already uses just work.)

I feel bad about requiring a special comment to silence the diagnostic.
There are too many things, that can go wrong here: e.g. we check the
placement of annotations and their correct spelling, should we do the same
with comments? Should we fail, if we meet a "// FALLTHROUGH" comment
somewhere outside of a switch statement? Should we allow "// FALL-THROUGH",
"// FALL THROUGH", "// break omitted intentionally", and all variations of
casing, spaces, additional clarifications, usage of /* */ comments etc?

Using comments for this purpose would be OK for a linter tool, but it seems
to be so wrong for a compiler diagnostics.

If one wants to enable the diagnostic, but is afraid of doing a global
cleanup prior to this, making Clang understand a certain form of the "//
FALLTHROUGH" comment won't help. I don't believe it can be used
consistently in the code base size of Chromium. And if it is used
consistently, the clean-up would be as easy, as search-replace + insert the
appropriate #include to all changed files (if there's a globally used
header, it's even easier).

#10

We may implement the solution you suggested. I can also take a look at
implementing a compiler built-in-based way to silence the diagnostic. I
guess, it's possible to restrict its usage to within switch statements
only, unlike the hack with the labels. What do you think?

The downside with a builtin is that any project that wants to use this
warning then needs to have a

#if __clang__ && __has_feature(__switch_fallthrough_macro_thinger)
#define FALLTHROUGH
__builtin_magic_which_annotates_fallthroughs_but_might_potentially_come_in_handy_in_other_contexts
#else
#define FALLTHROUGH
#endif

somewhere. If you have a central header that's included ~everywhere,
that's not a problem, but when your source is a collection of independent
modules not sharing much code, this gets much more annoying. (You could
arguably define this through a -D flag, but it's still less convenient that
having the comments that everybody already uses just work.)

I feel bad about requiring a special comment to silence the diagnostic.
There are too many things, that can go wrong here: e.g. we check the
placement of annotations and their correct spelling, should we do the same
with comments? Should we fail, if we meet a "// FALLTHROUGH" comment
somewhere outside of a switch statement?

No.

Should we allow "// FALL-THROUGH"

Not sure, probably not? Is that in use? I've never seen this.

, "// FALL THROUGH"

Yes (used frequently)

, "// break omitted intentionally"

No.

, and all variations of casing

Yes.

, spaces

Not all variations: I'd suggest one optional space between "fall" and
"through", and I'd ignore leading and trailing space.

, additional clarifications

No.

, usage of /* */ comments etc?

Sure, // and /* */ comments should both work.

The exact details don't really matter, pick something very simple, and
document this. For the code bases I looked at, the regex I proposed further
up ("fall", optional space, "through", optional punctuation sign, nothing
else in the comment, case-insensitive) covers the majority of fall-through
comments I've seen in practice (and the others aren't feasible to detect,
stuff like "// All fallthroughs in the following switch are intentional:").

Using comments for this purpose would be OK for a linter tool, but it
seems to be so wrong for a compiler diagnostics.

It makes the warning much more convenient to use, as it makes it work
out-of-the-box for code that's already annotation fallthroughs.

If one wants to enable the diagnostic, but is afraid of doing a global
cleanup prior to this,

I think nobody's opposed to doing a global cleanup. What is annoying having
to add a FALLTHROUGH macro in N places, and replacing "// FALLTHROUGH" with
"FALLTHROUGH;" seems like unnecessary busy work. (Especially given that
this change will probably make previously lint-clean code be no longer lint
clean.)