[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jun 16 00:26:36 PDT 2021


NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

This looks great to me, I think the patch is good to go as long as Valeriy's comment is addressed :)

Speaking of specs, hold up, we're forgetting something very important.

`make_unique()` also has to call the //constructor// of the underlying value. We're supposed to model that.

The constructor may be inlined or evaluated conservatively.

`~unique_ptr()` is also supposed to call the //destructor// of the underlying value. We're supposed to model that.

F17422410: aaaa.jpg <https://reviews.llvm.org/F17422410>

I.e., you could imagine a bunch of tests like this

  struct S {
    int *p;
    S(int *p): p(p) {}
    ~S() { *p = 0; }
  };
  
  void foo() {
    int x = 1;
    {
      auto P = std::make_unique<S>(&x);
      clang_analyzer_eval(*P->p == 1); // TRUE
    }
    clang_analyzer_eval(x == 0); // TRUE
  }

We could stick to conservative evaluation of such constructors and destructors; at the very least, we should actively invalidate Store at the destructor. This patch can remain unchanged because the default values inside freshly conjured regions are unknown. But that'd still be a regression because the above test (presumably) passes with inlining.

Is now a good time to legalize modeling function calls inside checker callbacks?



================
Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3
+// RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
+// RUN:  -analyzer-output=text -std=c++20 %s -verify=expected
----------------
Could you double-check that this flag correctly disables all the newly introduced modeling so that it wasn't accidentally enabled for all users before it's ready?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103750/new/

https://reviews.llvm.org/D103750



More information about the cfe-commits mailing list