False positive clang-analyzer-core.DivideZero with std::span

[chrchr-github]

struct A {
    explicit A(int& i) {
        data = std::span<int>{&i, 1};
    }
    void foo() const {
        for (auto& i : data)
                i++;
    }
    std::span<int> data;
};

int f() {
    int i = -1;
    A a{i};
    i = 0;
    a.foo();
    return 1 / i;
}

[<source>:18:14: warning: Division by zero [clang-analyzer-core.DivideZero]]
   18 |     return 1 / i;
      |              ^
[<source>:22:12: note: Calling 'f']
   22 |     return f();
      |            ^~~
[<source>:16:5: note: The value 0 is assigned to 'i']
   16 |     i = 0;
      |     ^~~~~
[<source>:18:14: note: Division by zero]
   18 |     return 1 / i;
      |            ~~^~~

https://godbolt.org/z/zzeMqGxev

The issue persists if foo() is non-const.

[llvmbot]

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

[mzyKi]

I think this false positive caused by clang-static-analyzer lack of std::span memory modeling.If we want to support for checking error in std::span,maybe need to realize one spanModeling like CStringChecker that evaluate c striing library functions.But now I don't enough knowledge to finish this work.

[haoNoQ]

I think span is a class we probably want to simply force-inline. It has no fancy aliasing-heavy data structures like red-black trees, and it's probably header-only.

Right now it probably falls under the "let's not dive too deep into the standard library" section, which is there specifically because we're not good at dealing with fancy aliasing-heavy data structures.

There's a few other issues with this report though, which are more tricky to handle. Just because we don't know what std::span does, doesn't mean we're allowed to have false positives.

In this case the bug happens on the execution path on which the loop for (auto& i : data) exits after zero iterations. This falls into #61669. Additionally, the analyzer fails to explain that zero iterations were taken; if it at least explained that part in path notes, the false positive would be a lot more understandable (albeit still unactionable and, well, false). This is a harder problem, because in order to recognize that this information is relevant we need to see what could have happened on the other branch (where the loop was executed one or more times) but didn't; this doesn't play well with our analysis technique that treats execution paths as completely independent.

Finally, path exploration aside, our individual bug path is also incorrect in isolation from other paths. After i is passed by non-const reference into to the (unfortunately) unknown constructor std::span<int>{&i, 1}, it becomes a "pre-escaped local" in the sense described in https://reviews.llvm.org/D71041. We should be treating any unknown function as potentially changing the value of that variable, as if it was a global variable. In particular, the function .begin() (implicitly called from the for-loop) should cause i to be invalidated, which didn't happen because we've never properly implemented pre-escaped locals.