Proposing bugprone-use-after-move Clang-tidy check

[Maksim Ivanov]

Hello everyone,

I'd like to propose enabling the Clang "bugprone-use-after-move" check for Chromium.

Examples of past bugs that were related to these "use-after-move" issues (and which, I believe, should be detectable by the bugprone-use-after-move check):

• crbug.com/409318
• crbug.com/614900
• crbug.com/714609
  
• crbug.com/733319
• crbug.com/746971
• crbug.com/868241
• crbug.com/876326
• crbug.com/956820
• crbug.com/995566
• crbug.com/1057309
• crbug.com/1068431

In accordance to the //docs/clang_tidy.md doc, I did an analysis of occurrences of this check throughout the current Chromium code base: https://docs.google.com/document/d/1n93Oq2l_0belsVSwekhdWutm5NKM9Y7KUaVTlGM1e80 . As described in that doc, there's a bunch of false positives, but also several real bugs found (even in the small sample of 30 checks).

P.S. The "bugprone-use-after-move" check does *not* catch all possible "use after move" scenarios. One of the biggest limitations is:

"The check currently only considers calls of std::move on local variables or function parameters. It does not check moves of member variables or global variables."

https://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html#move

However, I think that having this (imperfect) check enabled should anyway be better than not having it at all.

Thanks,

Maksim

[Peter Kasting]

+1!

PK

[Victor Costan]

+1, as a benefactor of this check in google3. IMO, having to tackle the false-positives is well worth the problems this catches.

Thank you,

    Victor

On Mon, Aug 24, 2020 at 7:26 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

+1!

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBe4nVsp8W6n8WX1VnHCvb859UxT5FiMidzGc18PxpOog%40mail.gmail.com.

[Maksim Ivanov]

Thanks! I've filed crbug.com/1122844 and will prepare CL(s) shortly.

[Maksim Ivanov]

Hello,

While still working on fixing the real/potential issues caught by this diagnostic, I noticed that sometimes clang-tidy signals about roughly the same problem using a different diagnostic name:

"clang-analyzer-cplusplus.Move"

I don't really understand in which cases this is emitted instead of "bugprone-use-after-move" (an example where "clang-analyzer-cplusplus.Move" is emitted is at https://source.chromium.org/chromium/chromium/src/+/master:third_party/perfetto/src/base/optional_unittest.cc;l=349;drc=919ca1eccdc38ea12a385918897c8de3c96c7deb).

Should we allowlist this "clang-analyzer-cplusplus.Move" as well, or is there a better way to deal with those not-well-documented aliases?

Maksim

[Nico Weber]

It looks like there are genuinely two distinct checkers for this upstream. One in clang's static analyzer, which is exposed via clang-tidy (the cplusplus.Move one), originally added in https://reviews.llvm.org/D24246, and a clang-tidy-only one, originally added in https://reviews.llvm.org/D54556

We should make sure we enable only one of them :)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DwB8xry5q2NXPM0ZgJ8EDEDd0wgkpaS-4kqeQfAYDj23du%2Bw%40mail.gmail.com.

[Maksim Ivanov]

Nico,

Thanks for the reply. But somehow these two checkers emit different results. AFAICS from the output of tricium_clang_tidy.py, there's no overlap between the warnings from one and the warnings from another.

Maksim