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

[NoQ]

Summary

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.

[Szelethus]

include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

929 ↗

(On Diff #168556)

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

[xazax.hun]

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.

[NoQ]

Whoops, almost forgot to doxygen-ize comments. Landed in rC344540.

[steakhal]

I'm not sure if this implementation is correct.

I'm expecting this checker code not to crash:

const auto *alloc = dyn_cast<CXXAllocatorCall>(&Call);
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 CallEvents directly, instead of using the underlaying CallExpr or CXXNewExpr objects in MallocChecker::checkCXXNewOrCXXDelete.
Am I misusing something? @NoQ

[NoQ]

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.

[steakhal]

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()?

[NoQ]

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.

[steakhal]

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?

[NoQ]

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.

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

public:
   virtual const CXXNewExpr *getOriginExpr() const {
     return cast<CXXNewExpr>(AnyFunctionCall::getOriginExpr());
   }
 
   const FunctionDecl *getDecl() const override {
     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; }
 
   static bool classof(const CallEvent *CE) {
     return CE->getKind() == CE_CXXAllocator;
   }
 };

cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp

static void addParameterValuesToBindings(const StackFrameContext *CalleeCtx,
   // do not bind any values to them.
   unsigned NumArgs = Call.getNumArgs();
   unsigned Idx = 0;
   ArrayRef<ParmVarDecl*>::iterator I = parameters.begin(), E = parameters.end();
   for (; I != E && Idx < NumArgs; ++I, ++Idx) {
     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));
       Bindings.push_back(std::make_pair(ParamLoc, ArgVal));
     }
   }
 
   // FIXME: Variadic arguments are not handled at all right now.

cfe/trunk/test/Analysis/new-aligned.cpp

+//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;
+}