[clang] False positive clang-analyzer-core.DivideZero with loop over container

[chrchr-github]

#include <vector>
int f(const std::vector<int>& v) {
    if (v.empty())
        return 0;
    int h = 0;
    for (auto it = v.begin(); it != v.end(); ++it) {
        h = std::max(h, *it);
    }

    int x = 512;
    if (h < x)
        h = h * (x / h + 1);
    return h;
}

<source>:12:20: warning: Division by zero [clang-analyzer-core.DivideZero]
   12 |         h = h * (x / h + 1);
      |                  ~~^~~
<source>:3:9: note: Assuming the condition is false]
    3 |     if (v.empty())
      |         ^~~~~~~~~
<source>:3:5: note: Taking false branch]
    3 |     if (v.empty())
      |     ^
<source>:5:5: note: 'h' initialized to 0
    5 |     int h = 0;
      |     ^~~~~
<source>:6:5: note: Loop condition is false. Execution continues on line 10
    6 |     for (auto it = v.begin(); it != v.end(); ++it) {
      |     ^
<source>:11:9: note: 'h' is < 'x'
   11 |     if (h < x)
      |         ^
<source>:11:5: note: Taking true branch
   11 |     if (h < x)
      |     ^
<source>:12:20: note: Division by zero
   12 |         h = h * (x / h + 1);
      |                  ~~^~~

There could be division by zero if the vector holds only numbers <= 0. However, the diagnostic seems to imply that the vector might be empty, which is impossible.
Edit: also happens with h = 1; instead of h = std::max(h, *it);.
https://godbolt.org/z/cbKh7axxd

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: None (chrchr-github)

~~~c++ #include <vector> int f(const std::vector<int>& v) { if (v.empty()) return 0; int h = 0; for (auto it = v.begin(); it != v.end(); ++it) { h = std::max(h, *it); }

int x = 512;
if (h < x)
    h = h * (x / h + 1);
return h;

}

<source>:12:20: warning: Division by zero [clang-analyzer-core.DivideZero]
12 | h = h * (x / h + 1);
| ^
<source>:3:9: note: Assuming the condition is false]
3 | if (v.empty())
| ^~~~~~~~~
<source>:3:5: note: Taking false branch]
3 | if (v.empty())
| ^
<source>:5:5: note: 'h' initialized to 0
5 | int h = 0;
| ^~~~~
<source>:6:5: note: Loop condition is false. Execution continues on line 10
6 | for (auto it = v.begin(); it != v.end(); ++it) {
| ^
<source>:11:9: note: 'h' is < 'x'
11 | if (h < x)
| ^
<source>:11:5: note: Taking true branch
11 | if (h < x)
| ^
<source>:12:20: note: Division by zero
12 | h = h * (x / h + 1);
| ^

There could be division by zero if the vector holds only numbers <= 0. However, the diagnostic seems to imply that the vector might be empty, which is impossible.
https://godbolt.org/z/cbKh7axxd
</details>

[mzyKi]

This is not a false positive about core.DivideZero.I think std::vector::empty() function related of this.I try to use this commandclang-tidy --allow-enabling-analyzer-alpha-checkers -checks=-*,clang-analyzer-core.DivideZero,clang-analyzer-alpha.cplusplus.IteratorModeling test.cpp -- -Xclang -analyzer-config -Xclang aggressive-binary-operation-simplification=trueand reproduce this.I guess we should fix the lack of empty() in IteratorModeling.

[NagyDonat]

You're right that the false positive is produced because the analyzer doesn't recognize connection between v.empty() and v.begin() != v.end(). (There is no stable checker that does this explicitly; and the implementation of these methods is either not available or too optimized/obfuscated for the analyzer.)

In an ideal world a checker like IteratorModeling would handle this issue; but unfortunately the code of the iterator-related checkers is infamously complex and difficult to modify. We know that they're somewhat buggy (that's why they're in alpha state) and it would be good to fix them eventually, but there are "lower hanging fruits" in other areas, so there is no progress on them.

[steakhal]

There is an easier option for this problem.
I've actually seen many instances of useful (and tiny) member functions not getting inlined for std::array (this was today for an other case I investigated downstream), but I've seen it happen for std::vector and std::span, std::string_view as well.

Anyways, there is an analyzer option called c++-container-inlining, which defaults to false, but if you set it, then CSA will not immediatelly reject inlining methods/functions from the standard library. I haven't looked in detail to the initial example, but setting this option would get rid of the div-by-zero FP. See https://godbolt.org/z/vo8MdMEf7

Downstream, we will definitely experiment with enabling this option, but one needs to be careful about checking if running times, memory footprint and the report changes improve / or are acceptable for the benefit.