Page MenuHomePhabricator

[clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable
ClosedPublic

Authored by ztamas on Mar 27 2019, 1:43 AM.

Details

Summary

The bugprone-too-small-loop-variable check often catches loop variables which can represent "big enough" values, so we don't actually need to worry about that this variable will overflow in a loop when the code iterates through a container. For example a 32 bit signed integer type's maximum value is 2 147 483 647 and a container's size won't reach this maximum value in most of the cases.
So the idea of this option to allow the user to specify an upper limit (using magnitude bit of the integer type) to filter out those catches which are not interesting for the user, so he/she can focus on the more risky integer incompatibilities.
Next to the option I replaced the term "positive bits" to "magnitude bits" which seems a better naming both in the code and in the name of the new option.

Diff Detail

Repository
rL LLVM

Event Timeline

ztamas created this revision.Mar 27 2019, 1:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2019, 1:43 AM

I run the check with and without the option on llvm source code. Without the option I found 370 catches, while setting MagnitudeBitsUpperLimit to 30 I found only two catches:

/home/zolnai/libreoffice/clang/llvm-project/llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp:390:32: warning: loop variable has narrower type 'uint16_t' (aka 'unsigned short') than iteration's upper bound 'uint32_t' (aka 'unsigned int') [bugprone-too-small-loop-variable]
  for (uint16_t StreamIdx = 0; StreamIdx < StreamCount; ++StreamIdx) {
                               ^

/home/zolnai/libreoffice/clang/llvm-project/llvm/tools/llvm-pdbutil/StreamUtil.cpp:98:32: warning: loop variable has narrower type 'uint16_t' (aka 'unsigned short') than iteration's upper bound 'uint32_t' (aka 'unsigned int') [bugprone-too-small-loop-variable]
  for (uint16_t StreamIdx = 0; StreamIdx < StreamCount; ++StreamIdx) {
                               ^

I found a project where the bugprone-too-small-loop-variable was used:
https://github.com/openMSX/openMSX/commit/55006063daffa90941d4c3f9b0034e494d9cf45a

As the commit message says for 32-bit integers the overflow never happens in practice.

Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
122 ↗(On Diff #192417)

:doc prefix and ` is missing. Please also rebase patch from trunk.

Eugene.Zelenko edited projects, added Restricted Project; removed Restricted Project.Mar 27 2019, 9:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2019, 9:29 AM

I think in general this is a good direction. What do you think about other heuristics?
E.g. one could say that a vector will not contain more then X GB or similar. That is probably more complicated to figure out though.

docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
44 ↗(On Diff #192417)

no warning for which value? 31? Please make that explicit in the doc.

ztamas marked an inline comment as done.Mar 27 2019, 11:34 PM

I think in general this is a good direction. What do you think about other heuristics?
E.g. one could say that a vector will not contain more then X GB or similar. That is probably more complicated to figure out though.

Yes, it would be very complicated to find out the memory size of the container used in the code, I think. The used size method is not always the size() function. In llvm code I see also std::string::length() and array_lengthof(). Also a code might use a non-standard container, which might have different method for getting the size of the container which makes it hard to create a matcher for this and find out which container I need to get it's size.

Also the check can catch loops which are not related to containers, so a container related option would have no meening in these cases.

I think it's the easiest way to specify the bits of the ineteger type to limit the catches. In real life, I met with this overflow / infinite loop problem with 16-bit short type, so I think the real use cases are 8 and 16 bit integers. It seems intuitive to me to use the size of the loop variable's type to separate those catches which can lead broken functionality in practice from those use cases which are just integer incompatibilities.

docs/ReleaseNotes.rst
122 ↗(On Diff #192417)

I'm a bit confused now about the code repositories, because of the recent changes. Which repo should I clone and rebase my commit?
Is this one the trunk?: http://llvm.org/svn/llvm-project/llvm/trunk

ztamas updated this revision to Diff 192582.Mar 28 2019, 2:14 AM

Rebased patch on https://github.com/llvm/llvm-project.git repo.
Updated the docs based on review comments.

ztamas marked 2 inline comments as done.Mar 28 2019, 2:16 AM

I think it's the easiest way to specify the bits of the ineteger type to limit the catches. In real life, I met with this overflow / infinite loop problem with 16-bit short type, so I think the real use cases are 8 and 16 bit integers. It seems intuitive to me to use the size of the loop variable's type to separate those catches which can lead broken functionality in practice from those use cases which are just integer incompatibilities.

Given your experience and the false positive rate in some projects, should we maybe default to 16 for that option?

I think it's the easiest way to specify the bits of the ineteger type to limit the catches. In real life, I met with this overflow / infinite loop problem with 16-bit short type, so I think the real use cases are 8 and 16 bit integers. It seems intuitive to me to use the size of the loop variable's type to separate those catches which can lead broken functionality in practice from those use cases which are just integer incompatibilities.

Given your experience and the false positive rate in some projects, should we maybe default to 16 for that option?

Seems a good idea. I'll update the code accordingly.

ztamas updated this revision to Diff 193017.Mar 31 2019, 9:22 AM

Make 16 the default value of MagnitudeBitsUpperLimit option.

JonasToth added inline comments.Apr 1 2019, 9:36 AM
clang-tools-extra/docs/ReleaseNotes.rst
124 ↗(On Diff #193017)

Please add the new default here as its a change in behaviour of the check.

clang-tools-extra/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
33 ↗(On Diff #193017)

typo magnitude

The commit you reference to the project fixing the "bugs" stated, that the version without conversions is more efficient (by one instruction :)). I think this might be worth a note, that even though the
code is not a bug in itself it might still be preferable to remove those code locations in the long run.

ztamas updated this revision to Diff 193277.Apr 2 2019, 6:26 AM

Fixed typo.
Mentioned the default value in release notes.

ztamas marked 3 inline comments as done.Apr 2 2019, 6:32 AM
ztamas added inline comments.
clang-tools-extra/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
33 ↗(On Diff #193017)

Typo is fixed

The referenced commit message mentions 32 bit system, where changing integer types was a bit faster. I'm not sure how general this is. I'm not familiar with the low level implementation of integer comparison, so I would not mention something, that I don't actually understand how it works.

ztamas marked an inline comment as done and an inline comment as not done.Apr 5 2019, 7:19 AM
JonasToth accepted this revision.Apr 7 2019, 5:23 AM

LGTM

clang-tools-extra/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst
33 ↗(On Diff #193017)

I dont understand it either in detail, it sounded logical that you need to convert the different integer sizes first somehow, but I am fine with not mentioning it :)

This revision is now accepted and ready to land.Apr 7 2019, 5:23 AM
ztamas updated this revision to Diff 194261.EditedApr 9 2019, 1:14 AM

Fixed a typo in release notes.
Rebased the patch withh git pull -r.

ztamas marked 2 inline comments as done.Apr 9 2019, 1:16 AM
ztamas added a comment.Apr 9 2019, 1:22 AM

Ok, I just fixed a remaining typo and rebased the patch to have it work with a more recent version of the code base, so I guess this patch is ready for commit.
Jonas, can you commit this change for me? I still don't have commit access, I just applied for it some days ago.

In the meantime, I've got the commit access, so I'll give it a try to push this myself.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2019, 5:48 AM