[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In https://reviews.llvm.org/D45532#1068700, @Szelethus wrote:

> In https://reviews.llvm.org/D45532#1068647, @dkrupp wrote:
>
> > This bug report also mentions assignment operator. But for that a warning may be not so useful. In that case the members of the assigned to object should have some initialized value already which the programmer may not want to overwrite in the assignment operator.
>
>
> I believe there's a checker for that already, but I'm really not sure whether `UndefinedAssignmentChecker` covers all such cases.


Indeed, there are two different targets for analysis here:

- The `rhs` of the assignment contains an uninitialised value, that is copied/moved into the `this` of the assignment. --> This //should// be checked by `UndefinedAssignmentChecker`.
- Not every field is initialised by the assignment operator. This one, I believe, is hard to check and prove, and also feels superfluous. As @NoQ wrote earlier, the amount of reports this checker emits in itself is, at least expected to be, huge, and while it can be a valid programming approach that a ctor must initialise all, tracking in the CSA on an `operator=` that the `this`-side had something uninitialised but it was also not initialised by the assignment? Hard, and also not very useful, as far as I see.

----

In https://reviews.llvm.org/D45532#1064652, @NoQ wrote:

> That said, your checker finds non-bugs, which is always risky. Sometimes the user should be allowed to initialize the field after construction but before the first use as a potentially important performance optimization. Did you find any such intentionally uninitialized fields with your checker? Were there many of those?


Where I can vision the usefulness of this checker **the most** is code that is evolving. There are many communities and codebases where coding standards and practices aren't laid out in a well-formed manner like LLVM has. There are also projects which are traditionally C code that has evolved into C++-ish code. When moving into C++, people start to realise they can write things like constructors, so they write them, but then leave out some members, and we can only guess (and **perhaps** make use of other checkers related to dereference or read of undefineds) what needs to be initialised, what is used without initialisation.

This checker is more close to something like a `bugprone-` Tidy matcher from the user's perspective, but proper analysis requires it to be executed in the SA.

A valid constriction of what this checker can find could be members that can "seemingly" be only be set in the constructor, there are no write operations or public exposure on them.

I have reviewed some findings of the checker, consider the following very trivial example:

  #define HEAD_ELEMENT_SPECIFIER 0xDEADF00D // some magic yadda
  
  struct linked_list;
  
  struct linked_list
  {
      int elem;
      linked_list* next;
  };
  
  class some_information_chain_class
  {
    public:
      some_information_chain_class() : iList(new linked_list())
      {
        iList->elem = HEAD_ELEMENT_SPECIFIER;
      }
  
    private:
      linked_list* iList;
  };

In this case, the "next" is in many cases remain as a garbage value. Of course, the checker cannot know, if, for example, there is also a `count` field and we don't rely on `next == nullptr` to signify the end of the list. But this can very well be a mistake from the programmer's end.

> If a user wants to have his codebase "analyzer-clean", would he be able to suppress the warning without changing the behavior of the program?

I'm not entirely sure what you mean by "changing the behaviour of the program". As far as I can see, this checker only reads information from the SA, so it should not mess up any preconception set or state potentially used by other checkers.

---

In https://reviews.llvm.org/D45532#1065716, @Szelethus wrote:

> I didn't implement anything explicitly to suppress warnings, so if there is nothing in the CSA by default for it, I'm afraid this isn't possible (just yet).


So far the only thing I can think of is coming up with new command-line arguments that can fine-tune the behaviour of the checker - but in this case, you can just as well just toggle the whole thing to be disabled.

An improvement heuristic (implemented in a later patch, toggleable with a command-line option perhaps?), as discussed above, would be checking //only// for members for whom there isn't any "setter" capability anywhere in the type.


https://reviews.llvm.org/D45532