[cfe-dev] Why these problems below were not found by Clang Static Analyzer?
Nico Weber
thakis at chromium.org
Mon Mar 17 04:17:53 PDT 2014
On Mon, Mar 17, 2014 at 10:55 AM, Alexander Kornienko <alexfh at google.com>wrote:
> 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?
>
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.)
> 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/a021c2b7/attachment.html>
More information about the cfe-dev
mailing list