I've made a blog post on http://googlerussiablog.blogspot.com about our bug finding tools and got a reply from PVS-Studio<http://www.viva64.com/ru/pvs-studio/> folks with a list of bugs they found in Chrome. (PVS-Studio is a commercial static analysis tool, they approach me from time to time). The list of bugs they posted looks very similar to what *clang* can detect. Am I right?
On Thu, May 19, 2011 at 7:33 AM, Kostya Serebryany <k...@google.com> wrote: > Hi, > I've made a blog post onšhttp://googlerussiablog.blogspot.comšabout our bug > finding tools and got a reply fromšPVS-Studiošfolks with a list of bugs they > found in Chrome. > (PVS-Studio is a commercial static analysis tool, they approach me from time > to time). > The list of bugs they posted looks very similar to what clang can detect. Am > I right? > Search for PVS inšhttp://habrahabr.ru/company/google/blog/119533/š(the > comments are in Russian, but the bug reports are in English) > --kcc
AFAICT, these are the issues mentioned in the blog post:
> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216 > #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 > bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) { > ... > info->is_directory = > file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0; > ... > }
Clang catches this with "warning: & has lower precedence than !=; != will be evaluated first" (-Wparentheses)
> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227 > void Time::Explode(bool is_local, Exploded* exploded) const { > ... > ZeroMemory(exploded, sizeof(exploded)); > ... > }
> ïÞÉÓÔÉÌÉ ÔÏÌØËÏ ÞÁÓÔØ ÂÕÆÅÒÁ.
Clang doesn't catch this.
> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404 > bool AutoFillProfileHasName(const AutoFillProfile& profile) { > return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || > !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || > !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); > }
> ä×Á ÒÁÚÁ NAME_MIDDLE ÉÓÐÏÌØÚÕÅÔÓÑ.
Clang doesn't catch this.
> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23 > IdleState CalculateIdleState(unsigned int idle_threshold) { > ... > DWORD current_idle_time = 0; > ... > // Will go -ve if we have been idle for a long time (2gb seconds). > if (current_idle_time < 0) > current_idle_time = INT_MAX; > ... > }
> äÏÌÖÎÏ ÂÙÔØ: > if (current_idle_time > INT_MAX)
Clang warns about this: "comparison of unsigned expression < 0 is always false" (-Wtautological-compare)
> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293 > bool NpProxyService::GetProxyValueJSONString(std::string* output) { > DCHECK(output); > output->empty(); > ... > }
> óÐÕÔÁÌÉ ÆÕÎËÃÉÀ clear() É empty().
I can't find the np_proxy_service.cc file, but I don't think Clang would warn here.
> V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400 > static const int kClientEdgeThickness; > int height() const; > bool ShouldShowClientEdge() const;
On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg <h...@chromium.org> wrote: >> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216 >> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 >> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) { >> š ... >> š info->is_directory = >> š š file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0; >> š ... >> }
>> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227 >> void Time::Explode(bool is_local, Exploded* exploded) const { >> š ... >> š ZeroMemory(exploded, sizeof(exploded)); >> š ... >> }
>> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404 >> bool AutoFillProfileHasName(const AutoFillProfile& profile) { >> š return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || >> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || >> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); >> }
>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23 >> IdleState CalculateIdleState(unsigned int idle_threshold) { >> š ... >> š DWORD current_idle_time = 0; >> š ... >> š // Will go -ve if we have been idle for a long time (2gb seconds). >> š if (current_idle_time < 0) >> š š current_idle_time = INT_MAX; >> š ... >> }
>> äÏÌÖÎÏ ÂÙÔØ: >> if (current_idle_time > INT_MAX)
> Clang warns about this: "comparison of unsigned expression < 0 is > always false" (-Wtautological-compare)
>> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293 >> bool NpProxyService::GetProxyValueJSONString(std::string* output) { >> š DCHECK(output); >> š output->empty(); >> š ... >> }
>> óÐÕÔÁÌÉ ÆÕÎËÃÉÀ clear() É empty().
> I can't find the np_proxy_service.cc file, but I don't think Clang > would warn here.
>> V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400 >> static const int kClientEdgeThickness; >> int height() const; >> bool ShouldShowClientEdge() const;
On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg <h...@chromium.org> wrote: > On Thu, May 19, 2011 at 7:33 AM, Kostya Serebryany <k...@google.com> wrote: >> Hi, >> I've made a blog post onšhttp://googlerussiablog.blogspot.comšabout our bug >> finding tools and got a reply fromšPVS-Studiošfolks with a list of bugs they >> found in Chrome. >> (PVS-Studio is a commercial static analysis tool, they approach me from time >> to time). >> The list of bugs they posted looks very similar to what clang can detect. Am >> I right? >> Search for PVS inšhttp://habrahabr.ru/company/google/blog/119533/š(the >> comments are in Russian, but the bug reports are in English) >> --kcc
> AFAICT, these are the issues mentioned in the blog post:
>> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216 >> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 >> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) { >> š ... >> š info->is_directory = >> š š file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0; >> š ... >> }
> Clang catches this with "warning: & has lower precedence than !=; != > will be evaluated first" (-Wparentheses)
>> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227 >> void Time::Explode(bool is_local, Exploded* exploded) const { >> š ... >> š ZeroMemory(exploded, sizeof(exploded)); >> š ... >> }
>> ïÞÉÓÔÉÌÉ ÔÏÌØËÏ ÞÁÓÔØ ÂÕÆÅÒÁ.
> Clang doesn't catch this.
clang's static analyzer on the other hand does I believe. The static analyzer started supporting C++ only very recently though, and it produces many many false positives at the moment, so that's not all that helpful.
>> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404 >> bool AutoFillProfileHasName(const AutoFillProfile& profile) { >> š return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || >> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || >> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); >> }
>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23 >> IdleState CalculateIdleState(unsigned int idle_threshold) { >> š ... >> š DWORD current_idle_time = 0; >> š ... >> š // Will go -ve if we have been idle for a long time (2gb seconds). >> š if (current_idle_time < 0) >> š š current_idle_time = INT_MAX; >> š ... >> }
>> äÏÌÖÎÏ ÂÙÔØ: >> if (current_idle_time > INT_MAX)
> Clang warns about this: "comparison of unsigned expression < 0 is > always false" (-Wtautological-compare)
>> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293 >> bool NpProxyService::GetProxyValueJSONString(std::string* output) { >> š DCHECK(output); >> š output->empty(); >> š ... >> }
>> óÐÕÔÁÌÉ ÆÕÎËÃÉÀ clear() É empty().
> I can't find the np_proxy_service.cc file, but I don't think Clang > would warn here.
I think clang _does_ support WARN_UNUSED_RESULT (see base/compiler_specific.h for the definition and base/file_path.h for example uses), just like gcc does. So both gcc and clang would've caught this.
>> V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400 >> static const int kClientEdgeThickness; >> int height() const; >> bool ShouldShowClientEdge() const;
On Thu, May 19, 2011 at 6:09 PM, Nico Weber <tha...@google.com> wrote: > On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg <h...@chromium.org> wrote: >> On Thu, May 19, 2011 at 7:33 AM, Kostya Serebryany <k...@google.com> wrote: >>> Hi, >>> I've made a blog post onšhttp://googlerussiablog.blogspot.comšabout our bug >>> finding tools and got a reply fromšPVS-Studiošfolks with a list of bugs they >>> found in Chrome. >>> (PVS-Studio is a commercial static analysis tool, they approach me from time >>> to time). >>> The list of bugs they posted looks very similar to what clang can detect. Am >>> I right? >>> Search for PVS inšhttp://habrahabr.ru/company/google/blog/119533/š(the >>> comments are in Russian, but the bug reports are in English) >>> --kcc
>> AFAICT, these are the issues mentioned in the blog post:
>>> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216 >>> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 >>> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) { >>> š ... >>> š info->is_directory = >>> š š file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0; >>> š ... >>> }
>> Clang catches this with "warning: & has lower precedence than !=; != >> will be evaluated first" (-Wparentheses)
>>> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227 >>> void Time::Explode(bool is_local, Exploded* exploded) const { >>> š ... >>> š ZeroMemory(exploded, sizeof(exploded)); >>> š ... >>> }
>>> ïÞÉÓÔÉÌÉ ÔÏÌØËÏ ÞÁÓÔØ ÂÕÆÅÒÁ.
>> Clang doesn't catch this.
> clang's static analyzer on the other hand does I believe. The static > analyzer started supporting C++ only very recently though, and it > produces many many false positives at the moment, so that's not all > that helpful.
>>> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404 >>> bool AutoFillProfileHasName(const AutoFillProfile& profile) { >>> š return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || >>> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || >>> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); >>> }
>>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23 >>> IdleState CalculateIdleState(unsigned int idle_threshold) { >>> š ... >>> š DWORD current_idle_time = 0; >>> š ... >>> š // Will go -ve if we have been idle for a long time (2gb seconds). >>> š if (current_idle_time < 0) >>> š š current_idle_time = INT_MAX; >>> š ... >>> }
>>> äÏÌÖÎÏ ÂÙÔØ: >>> if (current_idle_time > INT_MAX)
>> Clang warns about this: "comparison of unsigned expression < 0 is >> always false" (-Wtautological-compare)
>>> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293 >>> bool NpProxyService::GetProxyValueJSONString(std::string* output) { >>> š DCHECK(output); >>> š output->empty(); >>> š ... >>> }
>>> óÐÕÔÁÌÉ ÆÕÎËÃÉÀ clear() É empty().
>> I can't find the np_proxy_service.cc file, but I don't think Clang >> would warn here.
> I think clang _does_ support WARN_UNUSED_RESULT (see > base/compiler_specific.h for the definition and base/file_path.h for > example uses), just like gcc does. So both gcc and clang would've > caught this.
>>> V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400 >>> static const int kClientEdgeThickness; >>> int height() const; >>> bool ShouldShowClientEdge() const;
On Thu, May 19, 2011 at 9:29 AM, Evan Martin <e...@chromium.org> wrote: > On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg <h...@chromium.org> wrote: >>> V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216 >>> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 >>> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) { >>> š ... >>> š info->is_directory = >>> š š file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0; >>> š ... >>> }
>>> V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227 >>> void Time::Explode(bool is_local, Exploded* exploded) const { >>> š ... >>> š ZeroMemory(exploded, sizeof(exploded)); >>> š ... >>> }
>>> V501 There are identical sub-expressions to the left and to the right of the '||' operator. browser web_database.cc 404 >>> bool AutoFillProfileHasName(const AutoFillProfile& profile) { >>> š return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || >>> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || >>> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); >>> }
>>> ä×Á ÒÁÚÁ NAME_MIDDLE ÉÓÐÏÌØÚÕÅÔÓÑ.
> Can't find this code in our tree.
>>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23 >>> IdleState CalculateIdleState(unsigned int idle_threshold) { >>> š ... >>> š DWORD current_idle_time = 0; >>> š ... >>> š // Will go -ve if we have been idle for a long time (2gb seconds). >>> š if (current_idle_time < 0) >>> š š current_idle_time = INT_MAX; >>> š ... >>> }
>>> äÏÌÖÎÏ ÂÙÔØ: >>> if (current_idle_time > INT_MAX)
>> Clang warns about this: "comparison of unsigned expression < 0 is >> always false" (-Wtautological-compare)
>>> V530 The return value of function 'empty' is required to be utilized. chrome_frame_npapi np_proxy_service.cc 293 >>> bool NpProxyService::GetProxyValueJSONString(std::string* output) { >>> š DCHECK(output); >>> š output->empty(); >>> š ... >>> }
>>> óÐÕÔÁÌÉ ÆÕÎËÃÉÀ clear() É empty().
>> I can't find the np_proxy_service.cc file, but I don't think Clang >> would warn here.
> Can't find it either.
>>> V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400 >>> static const int kClientEdgeThickness; >>> int height() const; >>> bool ShouldShowClientEdge() const;
New findings from PVS folks (thanks Paul for keeping the watch). Should be interesting to both Chrome and Clang crowds. http://www.viva64.com/en/b/0113/
From the first glance, everything could be done in clang.
On Fri, May 20, 2011 at 4:46 PM, Nico Weber <tha...@chromium.org> wrote: > On Thu, May 19, 2011 at 9:29 AM, Evan Martin <e...@chromium.org> wrote: > > On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg <h...@chromium.org> > wrote: > >>> V564 The '&' operator is applied to bool type value. You've probably > forgotten to include parentheses or intended to use the '&&' operator. base > platform_file_win.cc 216 > >>> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 > >>> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) { > >>> ... > >>> info->is_directory = > >>> file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0; > >>> ... > >>> }
> >>> V512 A call of the 'memset' function will lead to underflow of the > buffer '(exploded)'. base time_win.cc 227 > >>> void Time::Explode(bool is_local, Exploded* exploded) const { > >>> ... > >>> ZeroMemory(exploded, sizeof(exploded)); > >>> ... > >>> }
> >>> V501 There are identical sub-expressions to the left and to the right > of the '||' operator. browser web_database.cc 404 > >>> bool AutoFillProfileHasName(const AutoFillProfile& profile) { > >>> return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || > >>> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || > >>> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); > >>> }
> >>> Два раза NAME_MIDDLE используется.
> > Can't find this code in our tree.
> >>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type > value is never < 0. browser idle_win.cc 23 > >>> IdleState CalculateIdleState(unsigned int idle_threshold) { > >>> ... > >>> DWORD current_idle_time = 0; > >>> ... > >>> // Will go -ve if we have been idle for a long time (2gb seconds). > >>> if (current_idle_time < 0) > >>> current_idle_time = INT_MAX; > >>> ... > >>> }
> >>> Должно быть: > >>> if (current_idle_time > INT_MAX)
> >> Clang warns about this: "comparison of unsigned expression < 0 is > >> always false" (-Wtautological-compare)
> >>> V530 The return value of function 'empty' is required to be utilized. > chrome_frame_npapi np_proxy_service.cc 293 > >>> bool NpProxyService::GetProxyValueJSONString(std::string* output) { > >>> DCHECK(output); > >>> output->empty(); > >>> ... > >>> }
> >>> Спутали функцию clear() и empty().
> >> I can't find the np_proxy_service.cc file, but I don't think Clang > >> would warn here.
> > Can't find it either.
> >>> V502 Perhaps the '?:' operator works in a different way than it was > expected. The '?:' operator has a lower priority than the '-' operator. > views custom_frame_view.cc 400 > >>> static const int kClientEdgeThickness; > >>> int height() const; > >>> bool ShouldShowClientEdge() const;
I am likely missing some context, but gcc -Wall has detected the first snippet below for several years now. Are the Chrome people not building with gcc, or do they not enable -Wall, or do they ignore warnings, or what?
Example:
mec@karp:~/exp-and$ cat z1.cc int Alpha(int a, int b) { return a & b != 0;
}
mec@karp:~/exp-and$ /usr/crosstool/v11/gcc-4.2.2-glibc-2.3.6-grte/x86_64-unknown-linux-gnu/bin/ x86_64-unknown-linux-gnu-g++ --version x86_64-unknown-linux-gnu-g++ (GCC) 4.2.2 Copyright (C) 2007 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
mec@karp:~/exp-and$ /usr/crosstool/v11/gcc-4.2.2-glibc-2.3.6-grte/x86_64-unknown-linux-gnu/bin/ x86_64-unknown-linux-gnu-g++ -Wall -Werror -c z1.cc cc1plus: warnings being treated as errors z1.cc: In function 'int Alpha(int, int)': z1.cc:2: warning: suggest parentheses around comparison in operand of &
> New findings from PVS folks (thanks Paul for keeping the watch). > Should be interesting to both Chrome and Clang crowds. > http://www.viva64.com/en/b/0113/
> From the first glance, everything could be done in clang.
> --kcc
> On Fri, May 20, 2011 at 4:46 PM, Nico Weber <tha...@chromium.org > <mailto:tha...@chromium.org>> wrote:
> On Thu, May 19, 2011 at 9:29 AM, Evan Martin <e...@chromium.org > <mailto:e...@chromium.org>> wrote: > > On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg > <h...@chromium.org <mailto:h...@chromium.org>> wrote: > >>> V564 The '&' operator is applied to bool type value. You've > probably forgotten to include parentheses or intended to use the > '&&' operator. base platform_file_win.cc 216 > >>> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 > >>> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* > info) { > >>> ... > >>> info->is_directory = > >>> file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0; > >>> ... > >>> }
> >>> V512 A call of the 'memset' function will lead to underflow of > the buffer '(exploded)'. base time_win.cc 227 > >>> void Time::Explode(bool is_local, Exploded* exploded) const { > >>> ... > >>> ZeroMemory(exploded, sizeof(exploded)); > >>> ... > >>> }
> >>> V501 There are identical sub-expressions to the left and to > the right of the '||' operator. browser web_database.cc 404 > >>> bool AutoFillProfileHasName(const AutoFillProfile& profile) { > >>> return > !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() ||
> >>> V547 Expression 'current_idle_time < 0' is always false. > Unsigned type value is never < 0. browser idle_win.cc 23 > >>> IdleState CalculateIdleState(unsigned int idle_threshold) { > >>> ... > >>> DWORD current_idle_time = 0; > >>> ... > >>> // Will go -ve if we have been idle for a long time (2gb > seconds). > >>> if (current_idle_time < 0) > >>> current_idle_time = INT_MAX; > >>> ... > >>> }
> >> I can't find the np_proxy_service.cc file, but I don't think Clang > >> would warn here.
> > Can't find it either.
> >>> V502 Perhaps the '?:' operator works in a different way than > it was expected. The '?:' operator has a lower priority than the > '-' operator. views custom_frame_view.cc 400 > >>> static const int kClientEdgeThickness; > >>> int height() const; > >>> bool ShouldShowClientEdge() const;
On Wed, Oct 19, 2011 at 10:06 PM, Kostya Serebryany <k...@google.com> wrote: > New findings from PVS folks (thanks Paul for keeping the watch). > Should be interesting to both Chrome and Clang crowds. > http://www.viva64.com/en/b/0113/
I saw this as well. There are a couple of Chrome folks slowly adding warnings they find useful to Clang and they've added a couple based on the PVS publishings.
> From the first glance, everything could be done in clang.
If there are specific warnings you want from Clang, please file bugs with examples. That way we can add them to the list, and plan appropriately to add them. =]
> On Fri, May 20, 2011 at 4:46 PM, Nico Weber <tha...@chromium.org> wrote:
>> On Thu, May 19, 2011 at 9:29 AM, Evan Martin <e...@chromium.org> wrote: >> > On Thu, May 19, 2011 at 3:44 AM, Hans Wennborg <h...@chromium.org> >> wrote: >> >>> V564 The '&' operator is applied to bool type value. You've probably >> forgotten to include parentheses or intended to use the '&&' operator. base >> platform_file_win.cc 216 >> >>> #define FILE_ATTRIBUTE_DIRECTORY 0x00000010 >> >>> bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) { >> >>> ... >> >>> info->is_directory = >> >>> file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0; >> >>> ... >> >>> }
>> >>> V512 A call of the 'memset' function will lead to underflow of the >> buffer '(exploded)'. base time_win.cc 227 >> >>> void Time::Explode(bool is_local, Exploded* exploded) const { >> >>> ... >> >>> ZeroMemory(exploded, sizeof(exploded)); >> >>> ... >> >>> }
>> >>> V501 There are identical sub-expressions to the left and to the right >> of the '||' operator. browser web_database.cc 404 >> >>> bool AutoFillProfileHasName(const AutoFillProfile& profile) { >> >>> return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || >> >>> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || >> >>> !profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); >> >>> }
>> >>> Два раза NAME_MIDDLE используется.
>> > Can't find this code in our tree.
>> >>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type >> value is never < 0. browser idle_win.cc 23 >> >>> IdleState CalculateIdleState(unsigned int idle_threshold) { >> >>> ... >> >>> DWORD current_idle_time = 0; >> >>> ... >> >>> // Will go -ve if we have been idle for a long time (2gb seconds). >> >>> if (current_idle_time < 0) >> >>> current_idle_time = INT_MAX; >> >>> ... >> >>> }
>> >>> Должно быть: >> >>> if (current_idle_time > INT_MAX)
>> >> Clang warns about this: "comparison of unsigned expression < 0 is >> >> always false" (-Wtautological-compare)
>> >>> V530 The return value of function 'empty' is required to be utilized. >> chrome_frame_npapi np_proxy_service.cc 293 >> >>> bool NpProxyService::GetProxyValueJSONString(std::string* output) { >> >>> DCHECK(output); >> >>> output->empty(); >> >>> ... >> >>> }
>> >>> Спутали функцию clear() и empty().
>> >> I can't find the np_proxy_service.cc file, but I don't think Clang >> >> would warn here.
>> > Can't find it either.
>> >>> V502 Perhaps the '?:' operator works in a different way than it was >> expected. The '?:' operator has a lower priority than the '-' operator. >> views custom_frame_view.cc 400 >> >>> static const int kClientEdgeThickness; >> >>> int height() const; >> >>> bool ShouldShowClientEdge() const;
On Wed, Oct 19, 2011 at 10:06 PM, Kostya Serebryany <k...@google.com> wrote: > New findings from PVS folks (thanks Paul for keeping the watch). > Should be interesting to both Chrome and Clang crowds.
>> >>> V512 A call of the 'memset' function will lead to underflow of the >> >>> buffer '(exploded)'. base time_win.cc 227 >> >>> void Time::Explode(bool is_local, Exploded* exploded) const { >> >>> š ... >> >>> š ZeroMemory(exploded, sizeof(exploded)); >> >>> š ... >> >>> }
>> >>> V501 There are identical sub-expressions to the left and to the right >> >>> of the '||' operator. browser web_database.cc 404 >> >>> bool AutoFillProfileHasName(const AutoFillProfile& profile) { >> >>> š return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || >> >>> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || >> >>> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); >> >>> }
>> >>> ä×Á ÒÁÚÁ NAME_MIDDLE ÉÓÐÏÌØÚÕÅÔÓÑ.
>> > Can't find this code in our tree.
>> >>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type >> >>> value is never < 0. browser idle_win.cc 23 >> >>> IdleState CalculateIdleState(unsigned int idle_threshold) { >> >>> š ... >> >>> š DWORD current_idle_time = 0; >> >>> š ... >> >>> š // Will go -ve if we have been idle for a long time (2gb seconds). >> >>> š if (current_idle_time < 0) >> >>> š š current_idle_time = INT_MAX; >> >>> š ... >> >>> }
>> >>> äÏÌÖÎÏ ÂÙÔØ: >> >>> if (current_idle_time > INT_MAX)
>> >> Clang warns about this: "comparison of unsigned expression < 0 is >> >> always false" (-Wtautological-compare)
>> >>> V530 The return value of function 'empty' is required to be utilized. >> >>> chrome_frame_npapi np_proxy_service.cc 293 >> >>> bool NpProxyService::GetProxyValueJSONString(std::string* output) { >> >>> š DCHECK(output); >> >>> š output->empty(); >> >>> š ... >> >>> }
>> >>> óÐÕÔÁÌÉ ÆÕÎËÃÉÀ clear() É empty().
>> >> I can't find the np_proxy_service.cc file, but I don't think Clang >> >> would warn here.
>> > Can't find it either.
>> >>> V502 Perhaps the '?:' operator works in a different way than it was >> >>> expected. The '?:' operator has a lower priority than the '-' operator. >> >>> views custom_frame_view.cc 400 >> >>> static const int kClientEdgeThickness; >> >>> int height() const; >> >>> bool ShouldShowClientEdge() const;
On Wed, Oct 19, 2011 at 10:14 PM, Michael Chastain <m...@google.com> wrote: > I am likely missing some context, but gcc -Wall has detected the first > snippet below for several years now. Are the Chrome people not building > with gcc, or do they not enable -Wall, or do they ignore warnings, or what?
Either way, if Clang isn't warning about that, it's worthy of a bug for us to go and add it. That code smells very very bad. =]
<ppluzhni...@google.com> wrote: > On Wed, Oct 19, 2011 at 10:06 PM, Kostya Serebryany <k...@google.com> wrote: >> New findings from PVS folks (thanks Paul for keeping the watch). >> Should be interesting to both Chrome and Clang crowds.
Yes, we added that. I added support for the same check in strncmp() in response to this current post. hwennborg added the ?: warning to clang a while ago.
We knew about several of the issues they published in this post already because compilers warned about them, but since it was in dead third-party code we haven't fixed it yet. (See http://crbug.com/100213 for the list).
>>> >>> V512 A call of the 'memset' function will lead to underflow of the >>> >>> buffer '(exploded)'. base time_win.cc 227 >>> >>> void Time::Explode(bool is_local, Exploded* exploded) const { >>> >>> š ... >>> >>> š ZeroMemory(exploded, sizeof(exploded)); >>> >>> š ... >>> >>> }
>>> >>> V501 There are identical sub-expressions to the left and to the right >>> >>> of the '||' operator. browser web_database.cc 404 >>> >>> bool AutoFillProfileHasName(const AutoFillProfile& profile) { >>> >>> š return !profile.GetFieldText(AutofillType(NAME_FIRST)).empty() || >>> >>> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty() || >>> >>> š š š š š!profile.GetFieldText(AutofillType(NAME_MIDDLE)).empty(); >>> >>> }
>>> >>> ä×Á ÒÁÚÁ NAME_MIDDLE ÉÓÐÏÌØÚÕÅÔÓÑ.
>>> > Can't find this code in our tree.
>>> >>> V547 Expression 'current_idle_time < 0' is always false. Unsigned type >>> >>> value is never < 0. browser idle_win.cc 23 >>> >>> IdleState CalculateIdleState(unsigned int idle_threshold) { >>> >>> š ... >>> >>> š DWORD current_idle_time = 0; >>> >>> š ... >>> >>> š // Will go -ve if we have been idle for a long time (2gb seconds). >>> >>> š if (current_idle_time < 0) >>> >>> š š current_idle_time = INT_MAX; >>> >>> š ... >>> >>> }
>>> >>> äÏÌÖÎÏ ÂÙÔØ: >>> >>> if (current_idle_time > INT_MAX)
>>> >> Clang warns about this: "comparison of unsigned expression < 0 is >>> >> always false" (-Wtautological-compare)
>>> >>> V530 The return value of function 'empty' is required to be utilized. >>> >>> chrome_frame_npapi np_proxy_service.cc 293 >>> >>> bool NpProxyService::GetProxyValueJSONString(std::string* output) { >>> >>> š DCHECK(output); >>> >>> š output->empty(); >>> >>> š ... >>> >>> }
>>> >>> óÐÕÔÁÌÉ ÆÕÎËÃÉÀ clear() É empty().
>>> >> I can't find the np_proxy_service.cc file, but I don't think Clang >>> >> would warn here.
>>> > Can't find it either.
>>> >>> V502 Perhaps the '?:' operator works in a different way than it was >>> >>> expected. The '?:' operator has a lower priority than the '-' operator. >>> >>> views custom_frame_view.cc 400 >>> >>> static const int kClientEdgeThickness; >>> >>> int height() const; >>> >>> bool ShouldShowClientEdge() const;
> I am likely missing some context, but gcc -Wall has detected the first > snippet below for several years now. Are the Chrome people not building > with gcc, or do they not enable -Wall, or do they ignore warnings, or what?
PVS Studio runs on Windows and targets the Windows builds. This is unfortunately one area where we lack good coverage. I understand that Google does/did build with Coverity, which offered Windows coverage, but Clang can not (yet) compile the Windows files and GCC on Windows for Chromium = (heh || meh).
As far as the Coverity builds, I'm not aware of anything going on for the non-Google Chromium development community, nor are there buildbots, so it's an area that can and does quickly/easily fall behind on - unlike the Mac/Linux side, which have clang buildbots building continuously and closing the tree on errors.
Additionally, the max warnings are only turned up when chromium_code=1 in the gyp files that control the builds. By default, third-party libs don't have this turned on - because unfortunately some of them play pretty fast and loose. From what I've seen from the PVS Studio people, most of their bugs tend to be reported against the third-party-deps of Chromium. Because they lump both Chromium code and third-party code together in their report, it's unsurprising to find that there may be missed coverage.
On Wed, Oct 19, 2011 at 10:17 PM, Chandler Carruth <chandl...@google.com> wrote: > On Wed, Oct 19, 2011 at 10:14 PM, Michael Chastain <m...@google.com> wrote:
>> I am likely missing some context, but gcc -Wall has detected the first >> snippet below for several years now. Are the Chrome people not building >> with gcc, or do they not enable -Wall, or do they ignore warnings, or what?
> Either way, if Clang isn't warning about that, it's worthy of a bug for us > to go and add it. That code smells very very bad. =]
This is http://code.google.com/p/chromium/issues/detail?id=83234 , filed 5 months ago and knows longer than that. Not fixed because it's dead code and the Real Fix is "figure out why this code is there at all and then delete it".
