2

For most of my C++ projects, I strongly rely on static analysis to prevent bugprone code getting into the master branch. However, nothing seems to detect bugs caused by unspecified order of evaluation. While some tools may spot suspicious code like foo(++i, --i), none of them seem to detect the problem here:

std::map<int, int> m;
int i = 0;
m[++i] = --i;

Here is the list of analyzers I've tried:

EDIT: as pointed out in the comments, the problem in the snippet above is detected by clang -std=c++14 -Wunsequenced. Here is a less trivial example that closely resembles a bug I've recently discovered in my project, and no warnings is given:

#include <unordered_map>
#include <cassert>
#include <memory>
#include <iostream>

struct Object {
};

struct Wrapper {
    std::unique_ptr<Object> object;
    std::string description;
};

class ObjectRegistry {
public:
    void add_object(Wrapper& w) {
        objects_[w.object.get()] = Wrapper{std::move(w.object), "added from " + w.description};

        // this assertion may or may not fail depending on the order of evaluation in the line above, which is unspecified
        assert(objects_.begin()->first != nullptr);
    }

private:
    // In our application, we need to quickly access the wrapper by raw pointer of its object
    std::unordered_map<Object*, Wrapper> objects_;
};

int main(int argc, char *argv[]) {
    ObjectRegistry registry;
    Wrapper w{std::make_unique<Object>(), "first object"};
    registry.add_object(w);
    return 0;
}

Is there any fundamental problem why this can't be detected with static analysis? If not, does there exist a tool that's able to detect it?

Roman Strakhov
  • 180
  • 9
  • 2
    Cmiiw but in recent standard, here `--i` is sequenced-before `++i` due to `operator=`. – lorro Jul 25 '22 at 19:20
  • 2
    @lorro Since C++17. Before it was unsequenced. – user17732522 Jul 25 '22 at 19:21
  • 3
    As such, `clang -std=c++14 -Wunsequenced` detects it: https://godbolt.org/z/WPhYvbcso. – Nate Eldredge Jul 25 '22 at 19:26
  • @NateEldredge good point! Well, the last snippet from the repo still isn't detected with `clang -std=c++14 -Wunsequenced`: https://godbolt.org/z/E935MYcae – Roman Strakhov Jul 25 '22 at 19:30
  • 1
    Because everything happens through function calls in the last snippet the operations are merely indeterminately sequenced even before C++17, not unsequenced (which is what `-Wunsequenced` warns about). Indeterminately-sequenced doesn't cause undefined behavior so it is less clear when to flag it. It would require deep analysis through multiple function calls and even then there might be false positives where the order doesn't really matter. – user17732522 Jul 25 '22 at 19:36
  • `Can` I do not think this is a programming question, it's more a question of "belief" I believe everything is possible - you can build a simulation of C++ abstract machine and detect anything you would want. `Is there any fundamental problem why this can't be detected with static analysis?`No, there is not. It's just hard. `If not, does there exist a tool that's able to detect it?` 3rd party tools recommendations are off topic for stackoverflow. Wiki has the list https://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis#C,_C++ , maybe one of them does. – KamilCuk Jul 28 '22 at 07:58
  • Aside: since C++20 you can have `std::unordered_set` and still have lookup by raw pointer. – Caleth Jul 28 '22 at 08:16

1 Answers1

1

As already mentioned, your first code snippet is correct since C++17 and it equals to m[0] = -1;. Before C++17 this code contains undefined behavior. I checked with PVS-Studio, and it issues the V567 warning.

Next example is really complex. Analyzer should do the following:

  • Know all the things about the order of evaluation for every version of the C++ standard. It's quite easy :)
  • Understand that Wrapper { .... } is an aggregate initialization. The first initializer will call move constructor of std::unique_ptr class for .object non-static data member
  • Understand that w.object was moved and the smart pointer becomes nullptr after the aggregate initialization
  • Understand that w.object.get() call will always return nullptr and issue some warning.

This is the behavior since C++17 and this code will always do strange thing. Before C++17 a compiler / static code analyzer could perform evaluation in any order.

Alexander Kurenev
  • 11
  • 1