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

[Nico Weber]

On Fri, Mar 14, 2014 at 1:08 PM, Richard Smith <richard at metafoo.co.uk>wrote:

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

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


> ----------------------------------------------------------------------
>> 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
>>
>>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140314/35243519/attachment.html>