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

[Nico Weber]

On Fri, Mar 14, 2014 at 4:16 PM, Alexander Kornienko <alexfh at google.com>wrote:

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


> On 14 Mar 2014 21:08, "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?
>>
>>> ----------------------------------------------------------------------
>>> 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/2203ab00/attachment.html>