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

[Alexander Kornienko]

On Sat, Mar 15, 2014 at 1:04 AM, Nico Weber <thakis at chromium.org> wrote:

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

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


>
>> 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
>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140317/1f5eae0d/attachment.html>