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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked an inline comment as done.
RedDocMD added inline comments.



Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:4
 // RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 

teemperor wrote:
> You set the standard here via the `-std` arg. If you want different standards 
> you need to have multiple `RUN:` lines that all pass a different standard.
Yup, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 351376.
RedDocMD added a comment.

Fixed up tests, now also runnning on C++20


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -1,3 +1,8 @@
+// RUN: %clang_analyze_cc1\
+// 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
+
 // RUN: %clang_analyze_cc1\
 // RUN:  -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\
 // RUN:  -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
@@ -313,3 +318,27 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202002L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite(); // expected-note {{std::unique_ptr 'P' constructed by std::make_unique_for_overwrite is null}}
+  *P;   // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1{{Dereference of null smart pointer 'P'}}
+}
+
+#endif
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -978,6 +978,17 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202002L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,6 +35,7 @@
 using namespace ento;
 
 namespace {
+
 class SmartPtrModeling
 : public Checker {
@@ -76,6 +77,9 @@
   {{"release"}, ::handleRelease},
   {{"swap", 1}, ::handleSwap},
   {{"get"}, ::handleGet}};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+  {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
@@ -137,12 +141,8 @@
 
 // Helper method to get the inner pointer type of specialized smart pointer
 // Returns empty type if not found valid inner pointer type.
-static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
-  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
-  if (!MethodDecl || !MethodDecl->getParent())
-return {};
-
-  const auto *RecordDecl = MethodDecl->getParent();
+static QualType getInnerPointerType(const CXXRecordDecl *RecordDecl,
+CheckerContext ) {
   if (!RecordDecl || !RecordDecl->isInStdNamespace())
 return {};
 
@@ -157,6 +157,17 @@
   return C.getASTContext().getPointerType(InnerValueType.getCanonicalType());
 }
 
+// Helper method to get the inner pointer type of specialized smart pointer
+// Returns empty type if not found valid inner pointer type.
+static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
+  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
+  if (!MethodDecl || !MethodDecl->getParent())
+return {};
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  return getInnerPointerType(RecordDecl, C);
+}
+
 // Helper method to pretty print region and avoid extra spacing.
 static void checkAndPrettyPrintRegion(llvm::raw_ostream ,
   const MemRegion *Region) {
@@ -177,7 +188,63 @@
 
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
   ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(StdMakeUniqueCall)) {
+const Optional 

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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

In D103750#2812613 , @RedDocMD wrote:

> How do I set the C++ standard while running a test?

If I understand your language correctly, then see my inline comment.




Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:4
 // RUN:  -analyzer-config 
cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\
 // RUN:  -analyzer-output=text -std=c++11 %s -verify=expected
 

You set the standard here via the `-std` arg. If you want different standards 
you need to have multiple `RUN:` lines that all pass a different standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

How do I set the C++ standard while running a test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 351356.
RedDocMD added a comment.

Added tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/Inputs/system-header-simulator-cxx.h
  clang/test/Analysis/smart-ptr-text-output.cpp

Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -313,3 +313,27 @@
 // expected-note@-1{{Dereference of null smart pointer 'P'}}
   }
 }
+
+void makeUniqueReturnsNonNullUniquePtr() {
+  auto P = std::make_unique();
+  if (!P) {   // expected-note {{Taking false branch}}
+P->foo(); // should have no warning here, path is impossible
+  }
+  P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}}
+  // Now P is null
+  if (!P) {
+// expected-note@-1 {{Taking true branch}}
+P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+// expected-note@-1{{Dereference of null smart pointer 'P'}}
+  }
+}
+
+#if __cplusplus >= 202003L
+
+void makeUniqueForOverwriteReturnsNullUniquePtr() {
+  auto P = std::make_unique_for_overwrite(); // expected-note {{std::unique_ptr 'uptr' constructed by std::make_unique_for_overwrite is null}}
+  *P;   // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+  // expected-note@-1{{Dereference of null smart pointer 'P'}}
+}
+
+#endif
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -978,6 +978,17 @@
 void swap(unique_ptr , unique_ptr ) noexcept {
   x.swap(y);
 }
+
+template 
+unique_ptr make_unique(Args &&...args);
+
+#if __cplusplus >= 202003L
+
+template 
+unique_ptr make_unique_for_overwrite();
+
+#endif
+
 } // namespace std
 #endif
 
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,6 +35,7 @@
 using namespace ento;
 
 namespace {
+
 class SmartPtrModeling
 : public Checker {
@@ -76,6 +77,9 @@
   {{"release"}, ::handleRelease},
   {{"swap", 1}, ::handleSwap},
   {{"get"}, ::handleGet}};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+  {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
@@ -137,12 +141,8 @@
 
 // Helper method to get the inner pointer type of specialized smart pointer
 // Returns empty type if not found valid inner pointer type.
-static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
-  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
-  if (!MethodDecl || !MethodDecl->getParent())
-return {};
-
-  const auto *RecordDecl = MethodDecl->getParent();
+static QualType getInnerPointerType(const CXXRecordDecl *RecordDecl,
+CheckerContext ) {
   if (!RecordDecl || !RecordDecl->isInStdNamespace())
 return {};
 
@@ -157,6 +157,17 @@
   return C.getASTContext().getPointerType(InnerValueType.getCanonicalType());
 }
 
+// Helper method to get the inner pointer type of specialized smart pointer
+// Returns empty type if not found valid inner pointer type.
+static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
+  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
+  if (!MethodDecl || !MethodDecl->getParent())
+return {};
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  return getInnerPointerType(RecordDecl, C);
+}
+
 // Helper method to pretty print region and avoid extra spacing.
 static void checkAndPrettyPrintRegion(llvm::raw_ostream ,
   const MemRegion *Region) {
@@ -177,7 +188,63 @@
 
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
   ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(StdMakeUniqueCall)) {
+const Optional ThisRegionOpt = Call.getReturnValueUnderConstruction();
+if (!ThisRegionOpt)
+  return false;
+
+const MemRegion *ThisRegion = ThisRegionOpt->getAsRegion();
+const TypedValueRegion *TVR = llvm::dyn_cast(ThisRegion);
+assert(TVR && "expected std::make_unique to return a std::unique_ptr "
+  "object (which is typed)");
+const QualType InnerPtrType =
+getInnerPointerType(TVR->getValueType()->getAsCXXRecordDecl(), C);
+

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 351349.
RedDocMD added a comment.

Put changes discussed in the meeting, tests to come in next revision


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,6 +35,7 @@
 using namespace ento;
 
 namespace {
+
 class SmartPtrModeling
 : public Checker {
@@ -76,6 +77,9 @@
   {{"release"}, ::handleRelease},
   {{"swap", 1}, ::handleSwap},
   {{"get"}, ::handleGet}};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+  {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
@@ -137,12 +141,8 @@
 
 // Helper method to get the inner pointer type of specialized smart pointer
 // Returns empty type if not found valid inner pointer type.
-static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
-  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
-  if (!MethodDecl || !MethodDecl->getParent())
-return {};
-
-  const auto *RecordDecl = MethodDecl->getParent();
+static QualType getInnerPointerType(const CXXRecordDecl *RecordDecl,
+CheckerContext ) {
   if (!RecordDecl || !RecordDecl->isInStdNamespace())
 return {};
 
@@ -157,6 +157,17 @@
   return C.getASTContext().getPointerType(InnerValueType.getCanonicalType());
 }
 
+// Helper method to get the inner pointer type of specialized smart pointer
+// Returns empty type if not found valid inner pointer type.
+static QualType getInnerPointerType(const CallEvent , CheckerContext ) {
+  const auto *MethodDecl = dyn_cast_or_null(Call.getDecl());
+  if (!MethodDecl || !MethodDecl->getParent())
+return {};
+
+  const auto *RecordDecl = MethodDecl->getParent();
+  return getInnerPointerType(RecordDecl, C);
+}
+
 // Helper method to pretty print region and avoid extra spacing.
 static void checkAndPrettyPrintRegion(llvm::raw_ostream ,
   const MemRegion *Region) {
@@ -177,7 +188,55 @@
 
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
   ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(StdMakeUniqueCall)) {
+const Optional ThisRegionOpt = Call.getReturnValueUnderConstruction();
+if (!ThisRegionOpt)
+  return false;
+
+const MemRegion *ThisRegion = ThisRegionOpt->getAsRegion();
+const TypedValueRegion *TVR = llvm::dyn_cast(ThisRegion);
+assert(TVR && "expected std::make_unique to return a std::unique_ptr "
+  "object (which is typed)");
+const QualType InnerPtrType =
+getInnerPointerType(TVR->getValueType()->getAsCXXRecordDecl(), C);
+assert(InnerPtrType.getTypePtr() &&
+   "expected to retrieve type of inner pointer of std::make_unique");
+const auto PtrVal = C.getSValBuilder().conjureSymbolVal(
+Call.getOriginExpr(), C.getLocationContext(), InnerPtrType,
+C.blockCount());
+
+State = State->set(ThisRegion, PtrVal);
+State = State->assume(PtrVal, true);
+
+auto  = State->getStateManager().getOwningEngine();
+State = Engine.updateObjectsUnderConstruction(
+*ThisRegionOpt, nullptr, State, C.getLocationContext(),
+Call.getConstructionContext(), {});
+
+C.addTransition(State);
+return true;
+  }
+
+  if (Call.isCalled(StdMakeUniqueForOverwriteCall)) {
+const Optional ThisRegionOpt = Call.getReturnValueUnderConstruction();
+if (!ThisRegionOpt)
+  return false;
+const MemRegion *ThisRegion = ThisRegionOpt->getAsRegion();
+const auto NullVal = C.getSValBuilder().makeNull();
+State = State->set(ThisRegion, NullVal);
+
+auto  = State->getStateManager().getOwningEngine();
+State = Engine.updateObjectsUnderConstruction(
+*ThisRegionOpt, nullptr, State, C.getLocationContext(),
+Call.getConstructionContext(), {});
+
+C.addTransition(State);
+return true;
+  }
+
   if (!smartptr::isStdSmartPtrCall(Call))
 return false;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D103750#2804438 , @RedDocMD wrote:

> Nice. Looks like I have something to read up on. Turns out, I end up learning 
> something new in C++ every now and then.  

Yeah, it's also pretty interesting ABI-wise. As you know, C++ doesn't have 
stable ABI. That said, one popular calling convention for functions that return 
C++ objects by value consists in passing a hidden argument to the function that 
contains the address at which to construct the return value. Like "this" is an 
implicit argument to methods, the return address is yet another implicit 
argument. So you can think of `getReturnValueUnderConstruction()` as of an 
accessor for that implicit argument value similarly to how `getCXXThisVal()` is 
the accessor for the implicit this-argument value.

This is completely different from C where you can return the structure on stack 
or even in registers (which can't be pointed to at all); this becomes possible 
in C because structures can be copied arbitrarily without invoking any sort of 
copy constructor.



Random general advice! When writing patches, you typically explore the design 
space of potential solutions - which you did great in this case. When you do 
so, it's a great idea to include the results of such exploration in the commit 
message (and/or in later comments). Especially if the first / most obvious 
thing to try didn't work. If you publish your second-choice solution, people 
will most likely ask you why you chose it instead of the obvious first-choice 
solution so it's an act of friendliness to anticipate this question. Similarly, 
if you have concerns about your solution, please communicate them openly; 
they'll either be discovered anyway but a lot more effort will be spent on it, 
or they won't be discovered during review which could result in future 
problems; both variants are worse than communicating openly. Ideally you carry 
more responsibility for proving that your approach is right than the reviewer 
has a responsibility to find problems in it. The reviewer is the last line of 
defense when it comes to applying critical thinking but the process really 
starts with you as you challenge your own assumptions and approaches by 
questioning both their correctness and their architectural harmony. Trying to 
find at least two solutions to each problem and compare them against each other 
is such a good exercise in critical thinking that almost every patch deserves 
such discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

Nice. Looks like I have something to read up on. Turns out, I end up learning 
something new in C++ every now and then.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Also congrats, sounds like you are to become the first actual user of this API! 
Hope it actually works 爛爛爛


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Whoops, I totally missed your point with my suggestion (should not read it 
while sitting at meetings). But Artem has a great answer already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D103750#2803516 , @RedDocMD wrote:

> In D103750#2803499 , @NoQ wrote:
>
>> Ugh, this entire `checkBind` hack was so unexpected that I didn't even 
>> recognize `evalCall`. What prevents you from doing everything in `evalCall`? 
>> No state traits, no nothing, just directly take the this-region and attach 
>> the value to it?
>
> Because in `evalCall`, I don't have access to the `ThisRegion`.

You do have access to it. That's how C++ works: the call constructs the value 
directly into the target this-region. In code

  auto a = std::make_unique(100)

it is known even before `std::make_unique` is invoked (i.e., even in 
`checkPreCall`) that the this-region for the call is going to be `a`. Because 
it's up to the call to invoke the constructor of the external object, and you 
can't invoke a constructor if you don't have a `this` to pass into it. //You 
cannot ever have an object and not have `this`.
//
Even if it wasn't for RVO, you would have known the temporary region in which 
the smart pointer would originally reside.

This was the whole point of the CFG work described in 
https://www.youtube.com/watch?v=4n3l-ZcDJNY

You're looking in the wrong place though, as `std::make_unique` returns the 
structure by value (aka prvalue), so the value of the expression, even if 
available before the call, was never going to be `this` (which would have been 
the corresponding glvalue). What you're looking for is 
`CallEvent::getReturnValueUnderConstruction()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D103750#2803516 , @RedDocMD wrote:

> In D103750#2803499 , @NoQ wrote:
>
>> Ugh, this entire `checkBind` hack was so unexpected that I didn't even 
>> recognize `evalCall`. What prevents you from doing everything in `evalCall`? 
>> No state traits, no nothing, just directly take the this-region and attach 
>> the value to it?
>
> Because in `evalCall`, I don't have access to the `ThisRegion`. At least I 
> think I don't.

Did you consider trying to get it from `CXXInstanceCall::getCXXThisVal`? 
Usually, if you cannot access something you need, that is a bug in the 
analyzer. But I found that this is very rarely the case. Most of the callbacks 
are very well thought out and already gives access to most of the information 
you might want to access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D103750#2803499 , @NoQ wrote:

> Ugh, this entire `checkBind` hack was so unexpected that I didn't even 
> recognize `evalCall`. What prevents you from doing everything in `evalCall`? 
> No state traits, no nothing, just directly take the this-region and attach 
> the value to it?

Because in `evalCall`, I don't have access to the `ThisRegion`. At least I 
think I don't.
So we have a statement like `auto a = std::make_unique(100)`. In 
`evalCall` I get a `CallEvent` corresponding to `std::make_unique(100)`. 
Using `Call.getOriginExpr()` I can get the LHS. But that's only the `Expr`, not 
the `MemRegion`.  Unlike in a regular constructor call or method call, where 
`ThisRegion` can easily be obtained from the `Call`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Ugh, this entire `checkBind` hack was so unexpected that I didn't even 
recognize `evalCall`. What prevents you from doing everything in `evalCall`? No 
state traits, no nothing, just directly take the this-region and attach the 
value to it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked an inline comment as done.
RedDocMD added a comment.

In D103750#2801555 , @NoQ wrote:

> Yes I think you should totally do an `evalCall()` here. The function has no 
> other side effects apart from making a pointer so it's valuable to fully 
> model it so that to avoid unnecessary store invalidations.

Am I not doing that now? I didn't get this point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked 3 inline comments as done.
RedDocMD added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:337
+
+bool isUniquePtrType(QualType QT) {
+  const auto *T = QT.getTypePtr();

NoQ wrote:
> Please merge with `isStdSmartPtrCall`.
I don't think that can be done because `isStdSmartPtrCall` checks //calls// 
like `a.foo()`, checking if `a` is a smart-ptr. On the other hand I need to 
check for statements like `a = foo()` and ensure that `a` is a smart-ptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 350362.
RedDocMD added a comment.

Made stylistic refactors


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,9 +35,15 @@
 using namespace ento;
 
 namespace {
+
+enum class MakeUniqueKind {
+  Regular,  // ie, std::make_unique
+  ForOverwrite, // ie, std::make_unique_for_overwrite
+};
+
 class SmartPtrModeling
 : public Checker {
+ check::LiveSymbols, check::Bind> {
 
   bool isBoolConversionMethod(const CallEvent ) const;
 
@@ -56,6 +62,7 @@
   void printState(raw_ostream , ProgramStateRef State, const char *NL,
   const char *Sep) const override;
   void checkLiveSymbols(ProgramStateRef State, SymbolReaper ) const;
+  void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext ) const;
 
 private:
   void handleReset(const CallEvent , CheckerContext ) const;
@@ -76,10 +83,27 @@
   {{"release"}, ::handleRelease},
   {{"swap", 1}, ::handleSwap},
   {{"get"}, ::handleGet}};
+  const CallDescription StdMakeUniqueCall{{"std", "make_unique"}};
+  const CallDescription StdMakeUniqueForOverwriteCall{
+  {"std", "make_unique_for_overwrite"}};
 };
 } // end of anonymous namespace
 
+class MakeUniqueKindWrapper {
+  const MakeUniqueKind Kind;
+
+public:
+  MakeUniqueKindWrapper(MakeUniqueKind Kind) : Kind(Kind) {}
+  MakeUniqueKind get() const { return Kind; }
+  void Profile(llvm::FoldingSetNodeID ) const {
+ID.AddInteger(static_cast(Kind));
+  }
+  bool operator==(const MakeUniqueKind ) const { return Kind == RHS; }
+  bool operator!=(const MakeUniqueKind ) const { return Kind != RHS; }
+};
+
 REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
+REGISTER_LIST_WITH_PROGRAMSTATE(MakeUniqueKindList, MakeUniqueKindWrapper)
 
 // Define the inter-checker API.
 namespace clang {
@@ -177,7 +201,21 @@
 
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
   ProgramStateRef State = C.getState();
+
+  if (Call.isCalled(StdMakeUniqueCall)) {
+State = State->add(MakeUniqueKind::Regular);
+C.addTransition(State);
+return true;
+  }
+
+  if (Call.isCalled(StdMakeUniqueForOverwriteCall)) {
+State = State->add(MakeUniqueKind::ForOverwrite);
+C.addTransition(State);
+return true;
+  }
+
   if (!smartptr::isStdSmartPtrCall(Call))
 return false;
 
@@ -272,6 +310,46 @@
   return C.isDifferent();
 }
 
+bool isUniquePtrType(QualType QT) {
+  if (QT.isNull())
+return false;
+  const auto *Decl = QT->getAsCXXRecordDecl();
+  if (!Decl || !Decl->getDeclContext()->isStdNamespace())
+return false;
+  const IdentifierInfo *ID = Decl->getIdentifier();
+  if (!ID)
+return false;
+  const StringRef Name = ID->getName();
+  return Name == "unique_ptr";
+}
+
+void SmartPtrModeling::checkBind(SVal L, SVal V, const Stmt *S,
+ CheckerContext ) const {
+  const auto *TVR = dyn_cast_or_null(L.getAsRegion());
+  if (!TVR)
+return;
+  if (!isUniquePtrType(TVR->getValueType()))
+return;
+  const auto *ThisRegion = dyn_cast(TVR);
+  if (!ThisRegion)
+return;
+
+  ProgramStateRef State = C.getState();
+  auto KindList = State->get();
+  assert(!KindList.isEmpty() &&
+ "Expected list to contain the kind of the last make_unique");
+  auto Kind = KindList.getHead();
+  if (Kind == MakeUniqueKind::ForOverwrite) {
+auto RHSVal = C.getSValBuilder().makeNull();
+State = State->set(ThisRegion, RHSVal);
+  } else {
+// TODO: Encode information that the inner pointer for
+// unique_ptr made by std::make_unique is *not* null
+  }
+  State = State->set(KindList.getTail());
+  C.addTransition(State);
+}
+
 void SmartPtrModeling::checkDeadSymbols(SymbolReaper ,
 CheckerContext ) const {
   ProgramStateRef State = C.getState();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Tests pls!

Yes I think you should totally do an `evalCall()` here. The function has no 
other side effects apart from making a pointer so it's valuable to fully model 
it so that to avoid unnecessary store invalidations.

In D103750#2801320 , @xazax.hun wrote:

>> Since we have CallExpr, we can easily conjure up an SVal. But I don't see 
>> how I can do it similarly in this patch.
>
> You should have a `CallExpr` for `std::make_unique` too. I believe that 
> expression is used to determine how the conjured symbol was created (to give 
> it an identity). So probably it should be ok to use the `make_unique` call to 
> create this symbol (but be careful to create the symbol with the right type).

That's right, a conjured symbol isn't necessarily the value of its respective 
expression. So you can conjure it and assume that it's non-null. Also consider 
using a //heap// conjured symbol (a-la `getConjuredHeapSymbolVal()`). That 
said, `getConjuredHeapSymbolVal()` doesn't provide an overload for overriding 
the type; there's no reason it shouldn't though, please feel free to extend it.

There's a separate story about `SymbolConjured` being the right class for the 
problem. I'm still in favor of having `SymbolMetadata` instead, so that to 
properly deduplicate symbols across different branches and merge more paths.




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:204
+MakeUniqueKind
+SmartPtrModeling::isStdMakeUniqueCall(const CallEvent ) const {
+  if (Call.getKind() != CallEventKind::CE_Function)

Please use `CallDescription` for most of this stuff.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:337
+
+bool isUniquePtrType(QualType QT) {
+  const auto *T = QT.getTypePtr();

Please merge with `isStdSmartPtrCall`.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:338-341
+  const auto *T = QT.getTypePtr();
+  if (!T)
+return false;
+  const auto *Decl = T->getAsCXXRecordDecl();

`getTypePtr()` is almost never necessary because `QualType` has an overloaded 
`operator->()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

> Since we have CallExpr, we can easily conjure up an SVal. But I don't see how 
> I can do it similarly in this patch.

You should have a `CallExpr` for `std::make_unique` too. I believe that 
expression is used to determine how the conjured symbol was created (to give it 
an identity). So probably it should be ok to use the `make_unique` call to 
create this symbol (but be careful to create the symbol with the right type).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 350110.
RedDocMD added a comment.

Fixed git history


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,11 +35,19 @@
 using namespace ento;
 
 namespace {
+
+enum class MakeUniqueKind {
+  Regular,  // ie, std::make_unique
+  ForOverwrite, // ie, std::make_unique_for_overwrite
+  None  // ie, is neither of the above two
+};
+
 class SmartPtrModeling
 : public Checker {
+ check::LiveSymbols, check::Bind> {
 
   bool isBoolConversionMethod(const CallEvent ) const;
+  MakeUniqueKind isStdMakeUniqueCall(const CallEvent ) const;
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
@@ -56,6 +64,7 @@
   void printState(raw_ostream , ProgramStateRef State, const char *NL,
   const char *Sep) const override;
   void checkLiveSymbols(ProgramStateRef State, SymbolReaper ) const;
+  void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext ) const;
 
 private:
   void handleReset(const CallEvent , CheckerContext ) const;
@@ -68,6 +77,8 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  void handleStdMakeUnique(const CallEvent , CheckerContext ,
+   MakeUniqueKind Kind) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) const;
@@ -79,7 +90,21 @@
 };
 } // end of anonymous namespace
 
+class MakeUniqueKindWrapper {
+  const MakeUniqueKind Kind;
+
+public:
+  MakeUniqueKindWrapper(MakeUniqueKind Kind) : Kind(Kind) {}
+  MakeUniqueKind get() const { return Kind; }
+  void Profile(llvm::FoldingSetNodeID ) const {
+ID.AddInteger(static_cast(Kind));
+  }
+  bool operator==(const MakeUniqueKind ) const { return Kind == RHS; }
+  bool operator!=(const MakeUniqueKind ) const { return Kind != RHS; }
+};
+
 REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
+REGISTER_LIST_WITH_PROGRAMSTATE(MakeUniqueKindList, MakeUniqueKindWrapper)
 
 // Define the inter-checker API.
 namespace clang {
@@ -175,8 +200,35 @@
   return CD && CD->getConversionType()->isBooleanType();
 }
 
+MakeUniqueKind
+SmartPtrModeling::isStdMakeUniqueCall(const CallEvent ) const {
+  if (Call.getKind() != CallEventKind::CE_Function)
+return MakeUniqueKind::None;
+  const auto *D = Call.getDecl();
+  if (!D)
+return MakeUniqueKind::None;
+  const auto *FTD = llvm::dyn_cast(D);
+  if (!FTD)
+return MakeUniqueKind::None;
+  if (FTD->getDeclName().isIdentifier()) {
+StringRef Name = FTD->getName();
+if (Name == "make_unique")
+  return MakeUniqueKind::Regular;
+else if (Name == "make_unique_for_overwrite")
+  return MakeUniqueKind::ForOverwrite;
+  }
+  return MakeUniqueKind::None;
+}
+
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
+  MakeUniqueKind Kind = isStdMakeUniqueCall(Call);
+  if (Kind != MakeUniqueKind::None) {
+handleStdMakeUnique(Call, C, Kind);
+return true;
+  }
+
   ProgramStateRef State = C.getState();
   if (!smartptr::isStdSmartPtrCall(Call))
 return false;
@@ -272,6 +324,59 @@
   return C.isDifferent();
 }
 
+void SmartPtrModeling::handleStdMakeUnique(const CallEvent ,
+   CheckerContext ,
+   MakeUniqueKind Kind) const {
+  assert(Kind != MakeUniqueKind::None &&
+ "Call is not to make_unique or make_unique_for_overwrite");
+  ProgramStateRef State = C.getState();
+  State = State->add(Kind);
+  C.addTransition(State);
+}
+
+bool isUniquePtrType(QualType QT) {
+  const auto *T = QT.getTypePtr();
+  if (!T)
+return false;
+  const auto *Decl = T->getAsCXXRecordDecl();
+  if (!Decl || !Decl->getDeclContext()->isStdNamespace())
+return false;
+  const IdentifierInfo *ID = Decl->getIdentifier();
+  if (!ID)
+return false;
+  const StringRef Name = ID->getName();
+  return Name == "unique_ptr";
+}
+
+void SmartPtrModeling::checkBind(SVal L, SVal V, const Stmt *S,
+ CheckerContext ) const {
+  const auto *TVR = dyn_cast_or_null(L.getAsRegion());
+  if (!TVR)
+return;
+  if (!isUniquePtrType(TVR->getValueType()))
+return;
+  const auto *ThisRegion = dyn_cast(TVR);
+  if (!ThisRegion)
+return;
+
+  ProgramStateRef State = C.getState();
+  auto KindList = State->get();
+  

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 350109.
RedDocMD added a comment.
Herald added a reviewer: bollu.

Reformatted code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  polly/include/polly/CodeGen/IslAst.h
  polly/include/polly/CodeGen/IslNodeBuilder.h
  polly/lib/CodeGen/IslAst.cpp
  polly/lib/CodeGen/IslNodeBuilder.cpp

Index: polly/lib/CodeGen/IslNodeBuilder.cpp
===
--- polly/lib/CodeGen/IslNodeBuilder.cpp
+++ polly/lib/CodeGen/IslNodeBuilder.cpp
@@ -300,12 +300,12 @@
 addReferencesFromStmtSet(Set, );
 }
 
-__isl_give isl_union_map *
-IslNodeBuilder::getScheduleForAstNode(__isl_keep isl_ast_node *For) {
-  return IslAstInfo::getSchedule(For);
+isl::union_map
+IslNodeBuilder::getScheduleForAstNode(const isl::ast_node ) {
+  return IslAstInfo::getSchedule(Node);
 }
 
-void IslNodeBuilder::getReferencesInSubtree(__isl_keep isl_ast_node *For,
+void IslNodeBuilder::getReferencesInSubtree(const isl::ast_node ,
 SetVector ,
 SetVector ) {
   SetVector SCEVs;
@@ -319,8 +319,7 @@
   for (const auto  : OutsideLoopIterations)
 Values.insert(cast(I.second)->getValue());
 
-  isl::union_set Schedule =
-  isl::manage(isl_union_map_domain(getScheduleForAstNode(For)));
+  isl::union_set Schedule = getScheduleForAstNode(For).domain();
   addReferencesFromStmtUnionSet(Schedule, References);
 
   for (const SCEV *Expr : SCEVs) {
@@ -476,22 +475,22 @@
   for (int i = 1; i < VectorWidth; i++)
 IVS[i] = Builder.CreateAdd(IVS[i - 1], ValueInc, "p_vector_iv");
 
-  isl_union_map *Schedule = getScheduleForAstNode(For);
-  assert(Schedule && "For statement annotation does not contain its schedule");
+  isl::union_map Schedule = getScheduleForAstNode(isl::manage_copy(For));
+  assert(!Schedule.is_null() &&
+ "For statement annotation does not contain its schedule");
 
   IDToValue[IteratorID] = ValueLB;
 
   switch (isl_ast_node_get_type(Body)) {
   case isl_ast_node_user:
-createUserVector(Body, IVS, isl_id_copy(IteratorID),
- isl_union_map_copy(Schedule));
+createUserVector(Body, IVS, isl_id_copy(IteratorID), Schedule.copy());
 break;
   case isl_ast_node_block: {
 isl_ast_node_list *List = isl_ast_node_block_get_children(Body);
 
 for (int i = 0; i < isl_ast_node_list_n_ast_node(List); ++i)
   createUserVector(isl_ast_node_list_get_ast_node(List, i), IVS,
-   isl_id_copy(IteratorID), isl_union_map_copy(Schedule));
+   isl_id_copy(IteratorID), Schedule.copy());
 
 isl_ast_node_free(Body);
 isl_ast_node_list_free(List);
@@ -504,7 +503,6 @@
 
   IDToValue.erase(IDToValue.find(IteratorID));
   isl_id_free(IteratorID);
-  isl_union_map_free(Schedule);
 
   isl_ast_node_free(For);
   isl_ast_expr_free(Iterator);
@@ -685,7 +683,7 @@
   SetVector SubtreeValues;
   SetVector Loops;
 
-  getReferencesInSubtree(For, SubtreeValues, Loops);
+  getReferencesInSubtree(isl::manage_copy(For), SubtreeValues, Loops);
 
   // Create for all loops we depend on values that contain the current loop
   // iteration. These values are necessary to generate code for SCEVs that
@@ -783,7 +781,7 @@
   bool Vector = PollyVectorizerChoice == VECTORIZER_POLLY;
 
   if (Vector && IslAstInfo::isInnermostParallel(isl::manage_copy(For)) &&
-  !IslAstInfo::isReductionParallel(For)) {
+  !IslAstInfo::isReductionParallel(isl::manage_copy(For))) {
 int VectorWidth = getNumberOfIterations(isl::manage_copy(For));
 if (1 < VectorWidth && VectorWidth <= 16 && !hasPartialAccesses(For)) {
   createForVector(For, VectorWidth);
@@ -791,12 +789,12 @@
 }
   }
 
-  if (IslAstInfo::isExecutedInParallel(For)) {
+  if (IslAstInfo::isExecutedInParallel(isl::manage_copy(For))) {
 createForParallel(For);
 return;
   }
-  bool Parallel =
-  (IslAstInfo::isParallel(For) && !IslAstInfo::isReductionParallel(For));
+  bool Parallel = (IslAstInfo::isParallel(isl::manage_copy(For)) &&
+   !IslAstInfo::isReductionParallel(isl::manage_copy(For)));
   createForSequential(isl::manage(For), Parallel);
 }
 
Index: polly/lib/CodeGen/IslAst.cpp
===
--- polly/lib/CodeGen/IslAst.cpp
+++ polly/lib/CodeGen/IslAst.cpp
@@ -140,7 +140,7 @@
 }
 
 /// Return all broken reductions as a string of clauses (OpenMP style).
-static const std::string getBrokenReductionsStr(__isl_keep isl_ast_node *Node) {
+static const std::string getBrokenReductionsStr(const isl::ast_node ) {
   IslAstInfo::MemoryAccessSet *BrokenReductions;
   std::string str;
 
@@ -171,25 +171,26 @@
 static isl_printer *cbPrintFor(__isl_take isl_printer *Printer,
__isl_take 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D103750#2801248 , @xazax.hun wrote:

> You can always create a new symbol to represent the inner pointer. Something 
> like this already happens, when you have a unique_ptr formal parameter and 
> call get on it.

The way `handleGet` obtains a new symbol cannot really be used here since we do 
not have an `Expr` for the inner pointer at hand. `handleGet` does the 
following:

  C.getSValBuilder().conjureSymbolVal(
  CallExpr, C.getLocationContext(), Call.getResultType(), 
C.blockCount());

Since we have `CallExpr`, we can easily conjure up an `SVal`. But I don't see 
how I can do it similarly in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.



> I am not being able to use this info since I don't have access to the raw 
> pointer, so cannot create a `SVal` and then constrain the `SVal` to non-null.
> Any suggestions @NoQ, @vsavchenko , @xazax.hun, @teemperor?

You can always create a new symbol to represent the inner pointer. Something 
like this already happens, when you have a unique_ptr formal parameter and call 
get on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

The drawback of the current approach is that we are not using the following 
piece of information:

> std::unique_ptr created from std::make_unique is **not** null (to begin with)

I am not being able to use this info since I don't have access to the raw 
pointer, so cannot create a `SVal` and then constrain the `SVal` to non-null.
Any suggestions @NoQ, @vsavchenko , @xazax.hun, @teemperor?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 350103.
RedDocMD added a comment.

Fixed binding of SVal to variable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,11 +35,19 @@
 using namespace ento;
 
 namespace {
+
+enum class MakeUniqueKind {
+  Regular, // ie, std::make_unique
+  ForOverwrite, // ie, std::make_unique_for_overwrite
+  None // ie, is neither of the above two
+};
+
 class SmartPtrModeling
 : public Checker {
+ check::LiveSymbols, check::Bind> {
 
   bool isBoolConversionMethod(const CallEvent ) const;
+  MakeUniqueKind isStdMakeUniqueCall(const CallEvent ) const;
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
@@ -56,6 +64,7 @@
   void printState(raw_ostream , ProgramStateRef State, const char *NL,
   const char *Sep) const override;
   void checkLiveSymbols(ProgramStateRef State, SymbolReaper ) const;
+  void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext ) const;
 
 private:
   void handleReset(const CallEvent , CheckerContext ) const;
@@ -68,6 +77,7 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  void handleStdMakeUnique(const CallEvent , CheckerContext , MakeUniqueKind Kind) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) const;
@@ -79,7 +89,21 @@
 };
 } // end of anonymous namespace
 
+class MakeUniqueKindWrapper {
+  const MakeUniqueKind Kind;
+
+public:
+  MakeUniqueKindWrapper(MakeUniqueKind Kind) : Kind(Kind) {}
+  MakeUniqueKind get() const { return Kind; }
+  void Profile(llvm::FoldingSetNodeID ) const {
+ID.AddInteger(static_cast(Kind));
+  }
+  bool operator==(const MakeUniqueKind ) const { return Kind == RHS; }
+  bool operator!=(const MakeUniqueKind ) const { return Kind != RHS; }
+};
+
 REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
+REGISTER_LIST_WITH_PROGRAMSTATE(MakeUniqueKindList, MakeUniqueKindWrapper)
 
 // Define the inter-checker API.
 namespace clang {
@@ -175,8 +199,34 @@
   return CD && CD->getConversionType()->isBooleanType();
 }
 
+MakeUniqueKind SmartPtrModeling::isStdMakeUniqueCall(const CallEvent ) const {
+  if (Call.getKind() != CallEventKind::CE_Function)
+return MakeUniqueKind::None;
+  const auto *D = Call.getDecl();
+  if (!D)
+return MakeUniqueKind::None;
+  const auto *FTD = llvm::dyn_cast(D);
+  if (!FTD)
+return MakeUniqueKind::None;
+  if (FTD->getDeclName().isIdentifier()) {
+StringRef Name = FTD->getName();
+if (Name == "make_unique")
+  return MakeUniqueKind::Regular;
+else if (Name == "make_unique_for_overwrite")
+  return MakeUniqueKind::ForOverwrite;
+  }
+  return MakeUniqueKind::None;
+}
+
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
+  MakeUniqueKind Kind = isStdMakeUniqueCall(Call);
+  if (Kind != MakeUniqueKind::None) {
+handleStdMakeUnique(Call, C, Kind);
+return true;
+  }
+
   ProgramStateRef State = C.getState();
   if (!smartptr::isStdSmartPtrCall(Call))
 return false;
@@ -272,6 +322,54 @@
   return C.isDifferent();
 }
 
+void SmartPtrModeling::handleStdMakeUnique(const CallEvent ,
+CheckerContext , MakeUniqueKind Kind) const {
+  assert(Kind != MakeUniqueKind::None && "Call is not to make_unique or make_unique_for_overwrite");
+  ProgramStateRef State = C.getState();
+  State = State->add(Kind);
+  C.addTransition(State);
+}
+
+bool isUniquePtrType(QualType QT) {
+  const auto *T = QT.getTypePtr();
+  if (!T)
+return false;
+  const auto *Decl = T->getAsCXXRecordDecl();
+  if (!Decl || !Decl->getDeclContext()->isStdNamespace())
+return false;
+  const IdentifierInfo *ID = Decl->getIdentifier();
+  if (!ID)
+return false;
+  const StringRef Name = ID->getName();
+  return Name == "unique_ptr";
+}
+
+void SmartPtrModeling::checkBind(SVal L, SVal V, const Stmt *S, CheckerContext ) const {
+  const auto *TVR = dyn_cast_or_null(L.getAsRegion());
+  if (!TVR)
+return;
+  if (!isUniquePtrType(TVR->getValueType()))
+return;
+  const auto *ThisRegion = dyn_cast(TVR);
+  if (!ThisRegion)
+return;
+
+  ProgramStateRef State = C.getState();
+  auto KindList = State->get();
+  assert(!KindList.isEmpty() && "Expected list to contain the kind of the last make_unique");
+  auto Kind = KindList.getHead();
+  

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 350067.
RedDocMD added a comment.

Accounting for std::make_unique_for_overwrite


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103750

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -35,11 +35,19 @@
 using namespace ento;
 
 namespace {
+
+enum class MakeUniqueKind {
+  Regular, // ie, std::make_unique
+  ForOverwrite, // ie, std::make_unique_for_overwrite
+  None // ie, is neither of the above two
+};
+
 class SmartPtrModeling
 : public Checker {
 
   bool isBoolConversionMethod(const CallEvent ) const;
+  MakeUniqueKind isStdMakeUniqueCall(const CallEvent ) const;
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
@@ -68,6 +76,7 @@
   bool updateMovedSmartPointers(CheckerContext , const MemRegion *ThisRegion,
 const MemRegion *OtherSmartPtrRegion) const;
   void handleBoolConversion(const CallEvent , CheckerContext ) const;
+  void handleMakeUnique(const CallEvent , CheckerContext , MakeUniqueKind Kind) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent , CheckerContext &) const;
@@ -175,8 +184,34 @@
   return CD && CD->getConversionType()->isBooleanType();
 }
 
+MakeUniqueKind SmartPtrModeling::isStdMakeUniqueCall(const CallEvent ) const {
+  if (Call.getKind() != CallEventKind::CE_Function)
+return MakeUniqueKind::None;
+  const auto *D = Call.getDecl();
+  if (!D)
+return MakeUniqueKind::None;
+  const auto *FTD = llvm::dyn_cast(D);
+  if (!FTD)
+return MakeUniqueKind::None;
+  if (FTD->getDeclName().isIdentifier()) {
+StringRef Name = FTD->getName();
+if (Name == "make_unique")
+  return MakeUniqueKind::Regular;
+else if (Name == "make_unique_for_overwrite")
+  return MakeUniqueKind::ForOverwrite;
+  }
+  return MakeUniqueKind::None;
+}
+
 bool SmartPtrModeling::evalCall(const CallEvent ,
 CheckerContext ) const {
+
+  MakeUniqueKind Kind = isStdMakeUniqueCall(Call);
+  if (Kind != MakeUniqueKind::None) {
+handleMakeUnique(Call, C, Kind);
+return true;
+  }
+
   ProgramStateRef State = C.getState();
   if (!smartptr::isStdSmartPtrCall(Call))
 return false;
@@ -214,7 +249,6 @@
   if (const auto *CC = dyn_cast()) {
 if (CC->getDecl()->isCopyConstructor())
   return false;
-
 const MemRegion *ThisRegion = CC->getCXXThisVal().getAsRegion();
 if (!ThisRegion)
   return false;
@@ -272,6 +306,33 @@
   return C.isDifferent();
 }
 
+void SmartPtrModeling::handleMakeUnique(const CallEvent ,
+CheckerContext , MakeUniqueKind Kind) const {
+  assert(Kind != MakeUniqueKind::None && "Call is not to make_unique or make_unique_for_overwrite");
+
+  const auto *OriginExpr = Call.getOriginExpr();
+  const auto *LocCtx = C.getLocationContext();
+
+  ProgramStateRef State = C.getState();
+
+  if (Kind == MakeUniqueKind::Regular) {
+  // With make unique it is not possible to make a null smart pointer.
+  // So we can set the SVal for Call.getOriginExpr() to be non-null
+  auto ExprVal = C.getSValBuilder().conjureSymbolVal(
+  OriginExpr, LocCtx, Call.getResultType(), C.blockCount());
+  State = State->assume(ExprVal, true);
+  State = State->BindExpr(OriginExpr, LocCtx, ExprVal);
+  } else {
+  // Technically, we are creating an uninitialized unique_ptr.
+  // So the value inside is *not* null, but we might as well set it
+  // so that the user gets better warnings.
+  auto ExprVal = C.getSValBuilder().makeZeroVal(Call.getResultType());
+  State = State->BindExpr(OriginExpr, LocCtx, ExprVal);
+  }
+
+  C.addTransition(State);
+}
+
 void SmartPtrModeling::checkDeadSymbols(SymbolReaper ,
 CheckerContext ) const {
   ProgramStateRef State = C.getState();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits