[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D52957#2383373 , @steakhal wrote:

> In D52957#2379330 , @NoQ wrote:
>
>> The argument value can be computed by taking the size of the type (and 
>> aligning to the requested alignment, i guess(?)) and multiplying it by array 
>> size (for which there is an expression) in case of array new. It'd be great 
>> to write down these computations once in the `CallEvent` class and then 
>> re-use them.
>
> Should I provide them as member functions to the `CXXAllocatorCall ` class?
> Something like `size_t getAlignment()` and `size_t getAllocationSize()`?

Yes. Note that allocation size is not necessarily concrete, so you'll have to 
return an `SVal` there. Alignment, i guess, is always concrete (?) you'll 
probably still want to return an `SVal` because a lot of users will want an 
`SVal` anyway.

>> I guess the actual shocking truth here is that we've never performed these 
>> computations when inlining the allocators; the size argument that's bound to 
>> the size parameter in the Store while the allocator body is inlined ended up 
>> being a fresh symbol, which is not correct.
>
> I might miss something to understand this. Could you elaborate on that if you 
> think is related?

For `CXXAllocatorCall`'s implicit arguments `getArgSVal()` always fails and 
returns an `UnknownVal` as it stumbles upon lack of expression and doesn't know 
how to work around it. In particular, it fails in 
`addParameterValuesToBindings()` which leaves the respective Store bindings 
empty.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52957

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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D52957#2379330 , @NoQ wrote:

> You cannot have an argument expression because there's no argument expression 
> anywhere in the AST. There's an argument, but it's not computed as a value of 
> any syntactic expression. If there was no argument, `getArgExpr(0)` would 
> have crashed; but it returns a `nullptr` which indicates that there's no 
> expression to return.

Aa, now I see. Thanks.

> The argument value can be computed by taking the size of the type (and 
> aligning to the requested alignment, i guess(?)) and multiplying it by array 
> size (for which there is an expression) in case of array new. It'd be great 
> to write down these computations once in the `CallEvent` class and then 
> re-use them.

Should I provide them as member functions to the `CXXAllocatorCall ` class?
Something like `size_t getAlignment()` and `size_t getAllocationSize()`?

> I guess the actual shocking truth here is that we've never performed these 
> computations when inlining the allocators; the size argument that's bound to 
> the size parameter in the Store while the allocator body is inlined ended up 
> being a fresh symbol, which is not correct.

I might miss something to understand this. Could you elaborate on that if you 
think is related?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52957

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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

You cannot have an argument expression because there's no argument expression 
anywhere in the AST. There's an argument, but it's not computed as a value of 
any syntactic expression. If there was no argument, `getArgExpr(0)` would have 
crashed; but it returns a `nullptr` which indicates that there's no expression 
to return.

The argument value can be computed by taking the size of the type (and aligning 
to the requested alignment, i guess(?)) and multiplying it by array size (for 
which there is an expression) in case of array new. It'd be great to write down 
these computations once in the `CallEvent` class and then re-use them.

I guess the actual shocking truth here is that we've never performed these 
computations when inlining the allocators; the size argument that's bound to 
the size parameter in the Store while the allocator body is inlined ended up 
being a fresh symbol, which is not correct.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52957

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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D52957#2377914 , @NoQ wrote:

> Everything looks good to me here. The new-expression `new int;` has 1 
> implicit argument (allocation size passed to the implementation of operator 
> new, the value is probably 4) and 0 placement arguments (the ones that are 
> explicitly written down after `new` and before `int`). See also 
> https://en.cppreference.com/w/cpp/language/new#Placement_new.

I'm still confused.

  alloc->getNumImplicitArgs(): 1
  alloc->getNumArgs(): 1
  alloc->getArgExpr(0): nullptr

I would expect `alloc->getArgExpr(0)` to return the //implicit// parameter, 
aka. the size of the placement new.
I can not see any other `getXX` function which seems applicable. My best bet 
was the `getPlacementArgExpr` thus I raised this issue here.

How can I access the placement new's allocation size of an `CXXAllocatorCall` 
if not via `getArgExpr()`?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52957

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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Everything looks good to me here. The new-expression `new int;` has 1 implicit 
argument (allocation size passed to the implementation of operator new, the 
value is probably 4) and 0 placement arguments (the ones that are explicitly 
written down after `new` and before `int`). See also 
https://en.cppreference.com/w/cpp/language/new#Placement_new.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52957

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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.
Herald added subscribers: ASDenysPetrov, martong, Charusso, dkrupp.

I'm not sure if this implementation is correct.

I'm expecting this checker code not to crash:

  const auto *alloc = dyn_cast();
  if (!alloc)
return;
  
  const int NumImpArgs = alloc->getNumImplicitArgs();
  errs() << "alloc->getNumImplicitArgs(): " << NumImpArgs << '\n'; // prints 1
  for (int i = 0; i < NumImpArgs; ++i)
errs() << "> " << alloc->getPlacementArgExpr(i) << '\n'; // crash: 
assertion violated
  
  const int NumArgs = alloc->getNumArgs();
  errs() << "alloc->getNumArgs(): " << NumArgs << '\n';
  for (int i = NumImpArgs; i < NumArgs; ++i)
errs() << "> " << alloc->getArgExpr(i) << '\n';

Analyzed code:

  void foo() {
int *p = new int;
  }

Assertion:

  clang: ../../clang/include/clang/AST/ExprCXX.h:2272: clang::Expr* 
clang::CXXNewExpr::getPlacementArg(unsigned int): Assertion `(I < 
getNumPlacementArgs()) && "Index out of range!"' failed.



---

I'm planning to improve the `MallocChecker` using `CallEvent`s directly, 
instead of using the underlaying `CallExpr` or `CXXNewExpr` objects in 
`MallocChecker::checkCXXNewOrCXXDelete`.
Am I misusing something? @NoQ


Repository:
  rL LLVM

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

https://reviews.llvm.org/D52957

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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Whoops, almost forgot to doxygen-ize comments. Landed in 
https://reviews.llvm.org/rC344540.


Repository:
  rL LLVM

https://reviews.llvm.org/D52957



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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL344539: [analyzer] Teach CallEvent about C++17 aligned 
operator new(). (authored by dergachev, committed by ).
Herald added subscribers: llvm-commits, donat.nagy.

Changed prior to commit:
  https://reviews.llvm.org/D52957?vs=168556=169726#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52957

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
  cfe/trunk/test/Analysis/new-aligned.cpp


Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -921,15 +921,28 @@
 return getOriginExpr()->getOperatorNew();
   }
 
+  // Size and maybe implicit alignment in C++17. Instead of size, the AST
+  // contains the construct-expression. Alignment is always hidden.
+  // We pretend that argument 0 is size and argument 1 is alignment (if passed
+  // implicitly) and the rest are placement args. This makes sure that the
+  // number of arguments is always the same as the number of parameters.
+  unsigned getNumImplicitArgs() const {
+return getOriginExpr()->passAlignment() ? 2 : 1;
+  }
+
   unsigned getNumArgs() const override {
-return getOriginExpr()->getNumPlacementArgs() + 1;
+return getOriginExpr()->getNumPlacementArgs() + getNumImplicitArgs();
   }
 
   const Expr *getArgExpr(unsigned Index) const override {
 // The first argument of an allocator call is the size of the allocation.
-if (Index == 0)
+if (Index < getNumImplicitArgs())
   return nullptr;
-return getOriginExpr()->getPlacementArg(Index - 1);
+return getOriginExpr()->getPlacementArg(Index - getNumImplicitArgs());
+  }
+
+  const Expr *getPlacementArgExpr(unsigned Index) const {
+return getOriginExpr()->getPlacementArg(Index);
   }
 
   Kind getKind() const override { return CE_CXXAllocator; }
Index: cfe/trunk/test/Analysis/new-aligned.cpp
===
--- cfe/trunk/test/Analysis/new-aligned.cpp
+++ cfe/trunk/test/Analysis/new-aligned.cpp
@@ -0,0 +1,14 @@
+//RUN: %clang_analyze_cc1 -std=c++17 -analyze -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+
+// Notice the weird alignment.
+struct alignas(1024) S {};
+
+void foo() {
+  // Operator new() here is the C++17 aligned new that takes two arguments:
+  // size and alignment. Size is passed implicitly as usual, and alignment
+  // is passed implicitly in a similar manner.
+  S *s = new S; // no-warning
+  delete s;
+}
Index: cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -503,10 +503,14 @@
 const ParmVarDecl *ParamDecl = *I;
 assert(ParamDecl && "Formal parameter has no decl?");
 
+// TODO: Support allocator calls.
 if (Call.getKind() != CE_CXXAllocator)
   if (Call.isArgumentConstructedDirectly(Idx))
 continue;
 
+// TODO: Allocators should receive the correct size and possibly alignment,
+// determined in compile-time but not represented as arg-expressions,
+// which makes getArgSVal() fail and return UnknownVal.
 SVal ArgVal = Call.getArgSVal(Idx);
 if (!ArgVal.isUnknown()) {
   Loc ParamLoc = SVB.makeLoc(MRMgr.getVarRegion(ParamDecl, CalleeCtx));


Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -921,15 +921,28 @@
 return getOriginExpr()->getOperatorNew();
   }
 
+  // Size and maybe implicit alignment in C++17. Instead of size, the AST
+  // contains the construct-expression. Alignment is always hidden.
+  // We pretend that argument 0 is size and argument 1 is alignment (if passed
+  // implicitly) and the rest are placement args. This makes sure that the
+  // number of arguments is always the same as the number of parameters.
+  unsigned getNumImplicitArgs() const {
+return getOriginExpr()->passAlignment() ? 2 : 1;
+  }
+
   unsigned getNumArgs() const override {
-return getOriginExpr()->getNumPlacementArgs() + 1;
+return getOriginExpr()->getNumPlacementArgs() + getNumImplicitArgs();
   }
 
   const Expr *getArgExpr(unsigned Index) const override {
 // The first argument of an allocator call is the size of the allocation.
-if (Index == 0)
+if (Index < getNumImplicitArgs())
   return nullptr;
-return getOriginExpr()->getPlacementArg(Index - 1);
+return 

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

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

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

LGTM!

I agree that it would make sense to either not have arguments that are not 
represented in the AST  or create expressions for those implicit arguments.


Repository:
  rC Clang

https://reviews.llvm.org/D52957



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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

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

Szelethus added inline comments.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:929
+  // number of arguments is always the same as the number of parameters.
+  unsigned getNumImplicitArgs() const {
+return getOriginExpr()->passAlignment() ? 2 : 1;

Can you include doxygen comments too, or make these doxygen comments?


Repository:
  rC Clang

https://reviews.llvm.org/D52957



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

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet, 
rnkovacs.
Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho, 
baloghadamsoftware.

In C++17, when class `C` has large alignment value, a special case of overload 
resolution rule kicks in for expression `new C` that causes the aligned version 
of `operator new()` to be called. The aligned `new` has two arguments: size and 
alignment. However, the new-expression has only one argument: the 
construct-expression for `C()`. This causes a false positive in 
`core.CallAndMessage`'s check for matching number of arguments and number of 
parameters.

Update `CXXAllocatorCall`, which is a `CallEvent` sub-class for operator new 
calls within new-expressions, so that the number of arguments always matched 
the number of parameters.

Dunno, maybe we should instead abandon the idea of reserving a whole 
argument/parameter index for each of those implicit arguments that aren't even 
represented by an expression in the AST.

Side note: Ugh, we never supported passing size into `operator new()` calls, 
even though it's known in compile time. And now also alignment. They are 
currently symbolic (unconstrained) within allocator calls.


Repository:
  rC Clang

https://reviews.llvm.org/D52957

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  lib/StaticAnalyzer/Core/CallEvent.cpp
  test/Analysis/new-aligned.cpp


Index: test/Analysis/new-aligned.cpp
===
--- /dev/null
+++ test/Analysis/new-aligned.cpp
@@ -0,0 +1,14 @@
+//RUN: %clang_analyze_cc1 -std=c++17 -analyze -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+
+// Notice the weird alignment.
+struct alignas(1024) S {};
+
+void foo() {
+  // Operator new() here is the C++17 aligned new that takes two arguments:
+  // size and alignment. Size is passed implicitly as usual, and alignment
+  // is passed implicitly in a similar manner.
+  S *s = new S; // no-warning
+  delete s;
+}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -503,10 +503,14 @@
 const ParmVarDecl *ParamDecl = *I;
 assert(ParamDecl && "Formal parameter has no decl?");
 
+// TODO: Support allocator calls.
 if (Call.getKind() != CE_CXXAllocator)
   if (Call.isArgumentConstructedDirectly(Idx))
 continue;
 
+// TODO: Allocators should receive the correct size and possibly alignment,
+// determined in compile-time but not represented as arg-expressions,
+// which makes getArgSVal() fail and return UnknownVal.
 SVal ArgVal = Call.getArgSVal(Idx);
 if (!ArgVal.isUnknown()) {
   Loc ParamLoc = SVB.makeLoc(MRMgr.getVarRegion(ParamDecl, CalleeCtx));
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -921,15 +921,28 @@
 return getOriginExpr()->getOperatorNew();
   }
 
+  // Size and maybe implicit alignment in C++17. Instead of size, the AST
+  // contains the construct-expression. Alignment is always hidden.
+  // We pretend that argument 0 is size and argument 1 is alignment (if passed
+  // implicitly) and the rest are placement args. This makes sure that the
+  // number of arguments is always the same as the number of parameters.
+  unsigned getNumImplicitArgs() const {
+return getOriginExpr()->passAlignment() ? 2 : 1;
+  }
+
   unsigned getNumArgs() const override {
-return getOriginExpr()->getNumPlacementArgs() + 1;
+return getOriginExpr()->getNumPlacementArgs() + getNumImplicitArgs();
   }
 
   const Expr *getArgExpr(unsigned Index) const override {
 // The first argument of an allocator call is the size of the allocation.
-if (Index == 0)
+if (Index < getNumImplicitArgs())
   return nullptr;
-return getOriginExpr()->getPlacementArg(Index - 1);
+return getOriginExpr()->getPlacementArg(Index - getNumImplicitArgs());
+  }
+
+  const Expr *getPlacementArgExpr(unsigned Index) const {
+return getOriginExpr()->getPlacementArg(Index);
   }
 
   Kind getKind() const override { return CE_CXXAllocator; }


Index: test/Analysis/new-aligned.cpp
===
--- /dev/null
+++ test/Analysis/new-aligned.cpp
@@ -0,0 +1,14 @@
+//RUN: %clang_analyze_cc1 -std=c++17 -analyze -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+
+// Notice the weird alignment.
+struct alignas(1024) S {};
+
+void foo() {
+  // Operator new() here is the C++17 aligned new that takes two arguments:
+  // size and alignment. Size is passed implicitly as usual, and alignment
+  // is passed implicitly in a