[Sema] Perform call checking when building CXXNewExpr

[lebedev.ri]

Summary

There was even a TODO for this.
The main motivation is to make use of call-site based
__attribute__((alloc_align(param_idx))) validation (D72996).

[lebedev.ri]

Ah right, hmm, posted too soon.
I actually need to synthesize the size/align arguments.

[lebedev.ri]

Not only do we need to 'insert' size implicit argument,
but we may need to insert alignment implicit argument.
And while for size we can only insert nullptr,
for alignment we can actually insert the correct value.

[lebedev.ri]

Rebased, NFC.

[lebedev.ri]

bump @rsmith

[rsmith]

It would be really useful to provide these extra parameters to FindAllocationFunctions too. For example, we could support direct dispatch to a size-class-specific allocation function:

void *operator new(std::size_t n) __attribute__((enable_if(n == 16, "optimized allocator for 16-byte objects")));

clang/lib/AST/ExprConstant.cpp

14464

This is wrong and may result in user-visible non-conformance; an expression of type std::align_val_t should never return true from isIntegerConstantExpr, because align_val_t is a scoped enumeration.

Can we instead change the caller (presumably the checking for the alloc_align attribute) to call a more general evaluation function rather than requiring an ICE?

clang/lib/Sema/SemaExprCXX.cpp

2084–2086

If this is not an array new, or it's an array new with a constant array bound, we can pass in a correct size. For the remaining (runtime array bound) cases, please conjure up an OpaqueValueExpr as the corresponding argument here rather than working around the nullptr argument in EvaluateWithSubstitution.

2095–2102

Please put these on the stack rather than ASTContext-allocating them. Both of these classes already support stack allocation.

[lebedev.ri]

@rsmith Thank you for taking a look!
Addressed all review comments.

[rsmith]

Looks good if you change the error to a warning.

clang/lib/Sema/SemaChecking.cpp

4482–4486

Oh, I wasn't aware we gave an error for this case. We can't issue an error for passing a bad alignment to operator new -- it's valid for code to pass a bad alignment, but results in undefined behavior. The same is true for aligned_alloc and posix_memalign, so we should presumably just downgrade this from an error to a warning in general rather than (say) treating operator new as a special case.

For example, this is valid and may be reasonable in some cases:

template<int N> void *make() {
  if ((N & (N-1)) == 0)
    return operator new(N, std::align_val_t(N));
  else
    return operator new(N);
}
void *p = make<7>();

[lebedev.ri]

clang/lib/Sema/SemaChecking.cpp

4482–4486

Hmm.
I'll post that as a followup patch here.

[lebedev.ri]

clang/lib/Sema/SemaChecking.cpp

4482–4486

Posted D73996.
I'm not sure which cases should remain as error?

[lebedev.ri]

Bump @rsmith
thanks

[lebedev.ri]

Bump @rsmith
thanks

[lebedev.ri]

Rebased ontop of patch demoting call-site-based align attr checking from error into a warning, NFC.

[lebedev.ri]

In D73020#1857704, @rsmith wrote:

Looks good if you change the error to a warning.

I'm going to treat that as an implicit accept since the error got changed into a warning.
Will commit this soon.

[lebedev.ri]

Rebased, NFC.

[rsmith]

In D73020#1891368, @lebedev.ri wrote:

In D73020#1857704, @rsmith wrote:

Looks good if you change the error to a warning.

I'm going to treat that as an implicit accept since the error got changed into a warning.

Yes, that was my intent. =) Here's an explicit "accept" for you anyway.

[lebedev.ri]

In D73020#1892245, @rsmith wrote:

In D73020#1891368, @lebedev.ri wrote:

In D73020#1857704, @rsmith wrote:

Looks good if you change the error to a warning.

I'm going to treat that as an implicit accept since the error got changed into a warning.

Yes, that was my intent. =) Here's an explicit "accept" for you anyway.

Great. Thank you for review!
Thank you for confirming.
Will land tomorrow.

clang/lib/AST/ExprConstant.cpp

#include "clang/AST/StmtNodes.inc"
   llvm_unreachable("Invalid StmtClass!");
 }
 
 /// Evaluate an expression as a C++11 integral constant expression.
 static bool EvaluateCPlusPlus11IntegralConstantExpr(const ASTContext &Ctx,
                                                     const Expr *E,
                                                     llvm::APSInt *Value,
                                                     SourceLocation *Loc) {
-  if (!E->getType()->isIntegralOrUnscopedEnumerationType()) {
-    if (Loc) *Loc = E->getExprLoc();
+  if (!E->getType()->isIntegralOrUnscopedEnumerationType() &&
+      !E->getType()->isAlignValT()) {
+    if (Loc)
+      *Loc = E->getExprLoc();
     return false;
   }
 
   APValue Result;
   if (!E->isCXX11ConstantExpr(Ctx, &Result, Loc))
     return false;
 
   if (!Result.isInt()) {
#endif
     // Ignore any side-effects from a failed evaluation. This is safe because
     // they can't interfere with any other argument evaluation.
     Info.EvalStatus.HasSideEffects = false;
   }
 
   ArgVector ArgValues(Args.size());
   for (ArrayRef<const Expr*>::iterator I = Args.begin(), E = Args.end();
        I != E; ++I) {
-    if ((*I)->isValueDependent() ||
+    // Arg can be null in malformed code.
+    if (!*I || (*I)->isValueDependent() ||
         !Evaluate(ArgValues[I - Args.begin()], Info, *I) ||
         Info.EvalStatus.HasSideEffects)
       // If evaluation fails, throw away the argument entirely.
       ArgValues[I - Args.begin()] = APValue();
 
     // Ignore any side-effects from a failed evaluation. This is safe because
     // they can't interfere with any other argument evaluation.
     Info.EvalStatus.HasSideEffects = false;

clang/lib/Sema/SemaChecking.cpp

if (FDecl) {
       for (const auto *I : FDecl->specific_attrs<ArgumentWithTypeTagAttr>())
         CheckArgumentWithTypeTag(I, Args, Loc);
     }
   }
 
   if (FDecl && FDecl->hasAttr<AllocAlignAttr>()) {
     auto *AA = FDecl->getAttr<AllocAlignAttr>();
     const Expr *Arg = Args[AA->getParamIndex().getASTIndex()];
-    if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
+    if (Arg && !Arg->isTypeDependent() && !Arg->isValueDependent()) {
       llvm::APSInt I(64);
       if (Arg->isIntegerConstantExpr(I, Context)) {
         if (!I.isPowerOf2()) {
           Diag(Arg->getExprLoc(), diag::err_alignment_not_power_of_two)
               << Arg->getSourceRange();
           return;
         }
 
         // Alignment calculations can wrap around if it's greater than 2**29.
         unsigned MaximumAlignment =
             536870912; // See llvm::Value::MaximumAlignment
         if (I > MaximumAlignment)
           Diag(Arg->getExprLoc(), diag::warn_assume_aligned_too_great)
               << Arg->getSourceRange() << MaximumAlignment;
       }

clang/lib/Sema/SemaExprCXX.cpp

if (OperatorNew) {
     if (GatherArgumentsForCall(PlacementLParen, OperatorNew, Proto,
                                PassAlignment ? 2 : 1, PlacementArgs,
                                AllPlaceArgs, CallType))
       return ExprError();
 
     if (!AllPlaceArgs.empty())
       PlacementArgs = AllPlaceArgs;
 
-    // FIXME: This is wrong: PlacementArgs misses out the first (size) argument.
-    DiagnoseSentinelCalls(OperatorNew, PlacementLParen, PlacementArgs);
+    llvm::SmallVector<Expr *, 8> CallArgs;
+    CallArgs.reserve(1 + PlacementArgs.size());
+    CallArgs.emplace_back(nullptr); // FIXME: size argument.
+    CallArgs.insert(CallArgs.end(), PlacementArgs.begin(), PlacementArgs.end());
 
-    // FIXME: Missing call to CheckFunctionCall or equivalent
+    DiagnoseSentinelCalls(OperatorNew, PlacementLParen, CallArgs);
+
+    checkCall(OperatorNew, Proto, /*ThisArg=*/nullptr, CallArgs,
+              /*IsMemberFunction=*/false, StartLoc, Range, CallType);
 
     // Warn if the type is over-aligned and is being allocated by (unaligned)
     // global operator new.
     if (PlacementArgs.empty() && !PassAlignment &&
         (OperatorNew->isImplicit() ||
          (OperatorNew->getBeginLoc().isValid() &&
           getSourceManager().isInSystemHeader(OperatorNew->getBeginLoc())))) {
       if (Alignment > NewAlignment)
         Diag(StartLoc, diag::warn_overaligned_type)
             << AllocType
             << unsigned(Alignment / Context.getCharWidth())
             << unsigned(NewAlignment / Context.getCharWidth());
     }
   }
 
   // Array 'new' can't have any initializers except empty parentheses.
   // Initializer lists are also allowed, in C++11. Rely on the parser for the
   // dialect distinction.
   if (ArraySize && !isLegalArrayNewInitializer(initStyle, Initializer)) {
     SourceRange InitRange(Inits[0]->getBeginLoc(),
                           Inits[NumInits - 1]->getEndLoc());

clang/test/SemaCXX/std-align-val-t-in-operator-new.cpp

 
 struct OVERALIGNED A {
   A();
   int n[128];
 };
 
 void *ptr_variable(int align) { return new (std::align_val_t(align)) A; }
 void *ptr_align16() { return new (std::align_val_t(16)) A; }
-void *ptr_align15() { return new (std::align_val_t(15)) A; }
+void *ptr_align15() { return new (std::align_val_t(15)) A; } // expected-error {{requested alignment is not a power of 2}}
 
 std::align_val_t align_variable(int align) { return std::align_val_t(align); }
 std::align_val_t align_align16() { return std::align_val_t(16); }
 std::align_val_t align_align15() { return std::align_val_t(15); }
-
-// expected-no-diagnostics