[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

14469 ↗

(On Diff #240196)

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

2130–2132

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.

2141–2148

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

3904–3910

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

3904–3910

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

[lebedev.ri]

clang/lib/Sema/SemaChecking.cpp

3904–3910

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/Sema/SemaChecking.cpp

if (FDecl) {
         CheckArgumentWithTypeTag(I, Args, Loc);
     }
   }
 
   if (FDecl && FDecl->hasAttr<AllocAlignAttr>()) {
     auto *AA = FDecl->getAttr<AllocAlignAttr>();
     const Expr *Arg = Args[AA->getParamIndex().getASTIndex()];
     if (!Arg->isValueDependent()) {
-      llvm::APSInt I(64);
-      if (Arg->isIntegerConstantExpr(I, Context)) {
+      Expr::EvalResult Align;
+      if (Arg->EvaluateAsInt(Align, Context)) {
+        const llvm::APSInt &I = Align.Val.getInt();
         if (!I.isPowerOf2())
           Diag(Arg->getExprLoc(), diag::warn_alignment_not_power_of_two)
               << Arg->getSourceRange();
 
         if (I > Sema::MaximumAlignment)
           Diag(Arg->getExprLoc(), diag::warn_assume_aligned_too_great)
               << Arg->getSourceRange() << Sema::MaximumAlignment;
       }
     }
   }
 
   if (FD)

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.
+
+    QualType SizeTy = Context.getSizeType();
+    unsigned SizeTyWidth = Context.getTypeSize(SizeTy);
+
+    llvm::APInt SingleEltSize(
+        SizeTyWidth, Context.getTypeSizeInChars(AllocType).getQuantity());
+
+    // How many bytes do we want to allocate here?
+    llvm::Optional<llvm::APInt> AllocationSize;
+    if (!ArraySize.hasValue() && !AllocType->isDependentType()) {
+      // For non-array operator new, we only want to allocate one element.
+      AllocationSize = SingleEltSize;
+    } else if (KnownArraySize.hasValue() && !AllocType->isDependentType()) {
+      // For array operator new, only deal with static array size case.
+      bool Overflow;
+      AllocationSize = llvm::APInt(SizeTyWidth, *KnownArraySize)
+                           .umul_ov(SingleEltSize, Overflow);
+      (void)Overflow;
+      assert(
+          !Overflow &&
+          "Expected that all the overflows would have been handled already.");
+    }
+
+    IntegerLiteral AllocationSizeLiteral(
+        Context,
+        AllocationSize.getValueOr(llvm::APInt::getNullValue(SizeTyWidth)),
+        SizeTy, SourceLocation());
+    // Otherwise, if we failed to constant-fold the allocation size, we'll
+    // just give up and pass-in something opaque, that isn't a null pointer.
+    OpaqueValueExpr OpaqueAllocationSize(SourceLocation(), SizeTy, VK_RValue,
+                                         OK_Ordinary, /*SourceExpr=*/nullptr);
+
+    // Let's synthesize the alignment argument in case we will need it.
+    // Since we *really* want to allocate these on stack, this is slightly ugly
+    // because there might not be a `std::align_val_t` type.
+    EnumDecl *StdAlignValT = getStdAlignValT();
+    QualType AlignValT =
+        StdAlignValT ? Context.getTypeDeclType(StdAlignValT) : SizeTy;
+    IntegerLiteral AlignmentLiteral(
+        Context,
+        llvm::APInt(Context.getTypeSize(SizeTy),
+                    Alignment / Context.getCharWidth()),
+        SizeTy, SourceLocation());
+    ImplicitCastExpr DesiredAlignment(ImplicitCastExpr::OnStack, AlignValT,
+                                      CK_IntegralCast, &AlignmentLiteral,
+                                      VK_RValue);
+
+    // Adjust placement args by prepending conjured size and alignment exprs.
+    llvm::SmallVector<Expr *, 8> CallArgs;
+    CallArgs.reserve(NumImplicitArgs + PlacementArgs.size());
+    CallArgs.emplace_back(AllocationSize.hasValue()
+                              ? static_cast<Expr *>(&AllocationSizeLiteral)
+                              : &OpaqueAllocationSize);
+    if (PassAlignment)
+      CallArgs.emplace_back(&DesiredAlignment);
+    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/diagnose_if.cpp

void run() {
     // FIXME: Same thing.
     for (void *p : adl::Bar(1)) {}
   }
 }
 
 namespace operator_new {
 struct Foo {
   int j;
-  static void *operator new(size_t i) _diagnose_if(i, "oh no", "warning");
+  static void *operator new(size_t i) _diagnose_if(i, "oh no", "warning"); // expected-note{{from 'diagnose_if'}}
 };
 
 struct Bar {
   int j;
   static void *operator new(size_t i) _diagnose_if(!i, "oh no", "warning");
 };
 
 void run() {
-  // FIXME: This should emit a diagnostic.
-  new Foo();
-  // This is here because we sometimes pass a dummy argument `operator new`. We
-  // should ignore this, rather than complaining about it.
+  new Foo(); // expected-warning{{oh no}}
   new Bar();
 }
 }
 
 namespace contextual_implicit_conv {
 struct Foo {
   int i;
   constexpr Foo(int i): i(i) {}

clang/test/SemaCXX/operator-new-size-diagnose_if.cpp

+// RUN: %clang_cc1 %s -verify -fno-builtin -std=c++14
+
+using size_t = decltype(sizeof(int));
+
+#define _diagnose_if(...) __attribute__((diagnose_if(__VA_ARGS__)))
+
+namespace operator_new {
+struct T0 {
+  int j = 0;
+  static void *operator new(size_t i) _diagnose_if(i == sizeof(int), "yay", "warning"); // expected-note{{from 'diagnose_if'}}
+};
+
+struct T1 {
+  int j = 0;
+  static void *operator new[](size_t i) _diagnose_if(i == 8 * sizeof(int), "yay", "warning"); // expected-note 2{{from 'diagnose_if'}}
+};
+
+void run(int x) {
+  new T0;       // expected-warning{{yay}}
+  new T1[8];    // expected-warning{{yay}}
+  new T1[4][2]; // expected-warning{{yay}}
+  new T1[x];    // no warning.
+}
+} // namespace operator_new

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-warning {{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-warning {{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