[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

2085–2087

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.

2096–2103

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

3887–3891 ↗

(On Diff #242373)

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

3887–3891 ↗

(On Diff #242373)

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

[lebedev.ri]

clang/lib/Sema/SemaChecking.cpp

3887–3891 ↗

(On Diff #242373)

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() ||
+    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/SemaExprCXX.cpp

const FunctionProtoType *Proto =
         OperatorNew->getType()->getAs<FunctionProtoType>();
     VariadicCallType CallType = Proto->isVariadic() ? VariadicFunction
                                                     : VariadicDoesNotApply;
 
     // We've already converted the placement args, just fill in any default
     // arguments. Skip the first parameter because we don't have a corresponding
     // argument. Skip the second parameter too if we're passing in the
     // alignment; we've already filled it in.
+    unsigned NumImplicitArgs = PassAlignment ? 2 : 1;
     if (GatherArgumentsForCall(PlacementLParen, OperatorNew, Proto,
-                               PassAlignment ? 2 : 1, PlacementArgs,
-                               AllPlaceArgs, CallType))
+                               NumImplicitArgs, 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);
+    // We would like to perform some checking on the given `operator new` call,
+    // but the PlacementArgs does not contain the implicit arguments,
+    // namely allocation size and maybe allocation alignment, so we need to
+    // conjure them. For size, we can't do anything better than just passing a
+    // nullptr, but for alignment we can either also pass a nullptr, or actually
+    // materialize the alignment we'll pass into the call.
+    llvm::SmallVector<Expr *, 8> CallArgs;
+    CallArgs.reserve(NumImplicitArgs + PlacementArgs.size());
+    CallArgs.emplace_back(nullptr);
+    if (PassAlignment) {
+      // Let's actually synthesize the alignment argument.
+      QualType AlignValT = Context.getTypeDeclType(getStdAlignValT());
+      QualType SizeTy =
+          AlignValT->castAs<EnumType>()->getDecl()->getIntegerType();
+      auto *AlignmentLiteral = IntegerLiteral::Create(
+          Context,
+          llvm::APInt(Context.getTypeSize(SizeTy),
+                      Alignment / Context.getCharWidth()),
+          SizeTy, SourceLocation());
+      auto *DesiredAlignmnet = ImplicitCastExpr::Create(
+          Context, AlignValT, CK_IntegralCast, AlignmentLiteral,
+          /*BasePath=*/nullptr, VK_RValue);
+      CallArgs.emplace_back(DesiredAlignmnet);
+    }
+    CallArgs.insert(CallArgs.end(), PlacementArgs.begin(), PlacementArgs.end());
+
+    DiagnoseSentinelCalls(OperatorNew, PlacementLParen, CallArgs);
 
-    // FIXME: Missing call to CheckFunctionCall or equivalent
+    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)

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}}
 
 struct alignas(128) S {
   S() {}
 };
 
 void *alloc_overaligned_struct() {
   return new S;
 }
 
 void *alloc_overaligned_struct_with_extra_variable_alignment(int align) {
   return new (std::align_val_t(align)) S;
 }
 void *alloc_overaligned_struct_with_extra_256_alignment(int align) {
   return new (std::align_val_t(256)) S;
 }
 void *alloc_overaligned_struct_with_extra_255_alignment(int align) {
-  return new (std::align_val_t(255)) S;
+  return new (std::align_val_t(255)) S; // 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