[PATCH] D54557: [analyzer] MoveChecker Pt.2: Restrict the warning to STL objects and locals.

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

In https://reviews.llvm.org/D54557#1300581, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:
>
> > Whenever I compile `Rtags`, I get a bunch of move related warnings, I'm curious how this patch behaves on that project. I'll take a look.
>
>
> Whoops, sry, i accidentally had a look because i misread your message as if you wanted me to take a look >.<
>
> (IMPORTANT) **Spoiler alert!**
>
> It finds one positive (and one duplicate of that one positive):
>
> F7553145: rtags-move-positive.html <https://reviews.llvm.org/F7553145>
>
> I believe this positive is a real bug, but we can still do better here. We find it as a use-after-move of a local variable, which is not a bug on its own, i.e. the user intended to empty and then re-use the storage and it's fine, this is not a usual kind of typo where the user uses the pre-move variable instead of the post-move variable. But the real bug here is that this local variable uses an `std::string` as a field under the hood, which is, well, not guaranteed to be empty after move, like all other STL objects: https://stackoverflow.com/questions/27376623/can-you-reuse-a-moved-stdstring
>
> NOTE: As also mentioned in this stackoverflow thread, we also need to exclude smart pointers from the STL check because they don't end up in unspecified state, and see if there are other cornercases here.


OMG That is cool. :D That project was fishy. Nice catch, would you like to make an issue on their project or shall I?

> Unfortunately, we don't find this bug as an STL use-after-move bug because inlining isn't happening. Why? I guess i'll leave it as an excercise ^.^ It's a combination of running out of budget and a specific feature that we have. By flipping a single `-analyzer-config` flag that represents that feature, we are able to find such bugs as STL bugs (when local variable bugs are disabled in the checker). We're still not able to find the original bug, most likely due to running out of budget (i didn't debug this further), but we can find it in a minimal example:
> 
>   #include "rct/Rct.h"
>   
>   void foo() {
>     String S1;
>     String S2 = std::move(S1);
>     S1 += "asdfg"; // use-after-move of a std::string
>   }
> 
> 
> Here's the report that we are able to obtain for this trivial code snippet, and you can look up the answer to the exercise in the collapsed run-line :)
> 
> F7553290: rtags-move-positive-simplified.html <https://reviews.llvm.org/F7553290>

Thanks! :) *throws confetti*

In https://reviews.llvm.org/D54557#1300653, @NoQ wrote:

> In https://reviews.llvm.org/D54557#1299899, @Szelethus wrote:
>
> > I think we should either remove the non-default functionality (which wouldn't be ideal), or emphasise somewhere (open projects?) that there is still work to be done, but leaving it to be forgotten and essentially making it an extra maintenance work would be, in my optinion, the worst case scenario. `Aggressive` isn't `Pedantic` because it actually emits warnings on correct code, and it's not a simple matter of too many reports being emitted, let's also document that this is an experimental feature, not a power-user-only thing.
>
>
> I only kept the option around because i was under an impression that i'm intruding into a checker that already has some happy users, probably breaking existing workflows. If this option is unnecessary, i'd be happy to remove it :)


Hmm, I'll ask around, but I'm not aware of any ongoing (or planned in the near future) work on this particular checker.


Repository:
  rC Clang

https://reviews.llvm.org/D54557