[Clang] Check for returns_nonnull when deciding to add allocation null checks

[modimo]

Summary

Non-throwing allocators currently will always get null-check code. However, if the non-throwing allocator is explicitly annotated with returns_nonnull the null check should be elided.

Testing:
ninja check-all
added test case correctly elides

[modimo]

Go back to single statement return, fix up ATTR_BUILTIN_NOTHROW_NEW test label that pointed to nothing to correct ATTR_NOBUILTIN_NOUNWIND_ALLOCSIZE label.

[modimo]

Discussing with @urnathan this makes more sense for the BE to handle when optimizing by eliding the generated null check. Confirmed this is indeed the case so removing the generation in the FE isn't really needed.

[bruno]

Sounds reasonable to me! Can you double check whether this attribute gets correctly serialized/deserialized in face of CXXNewExpr? An example of how to test that would be in clang/test/PCH/cxx-method.cpp.

[modimo]

In D102820#2772184, @bruno wrote:

Sounds reasonable to me! Can you double check whether this attribute gets correctly serialized/deserialized in face of CXXNewExpr? An example of how to test that would be in clang/test/PCH/cxx-method.cpp.

Piggybacking on that test case:

Inputs/cxx-method.h:

typedef __typeof__(sizeof(0)) size_t;

void *operator new(size_t size)
{
  return ::operator new(size);
}

cxx-method.cpp:

int * foo()
{
    return new int;
}

Testing

~/llvm-project/clang/test/PCH# ~/llvm-project/build-rel/bin/clang++ -cc1 -x c++  -emit-pch Inputs/cxx-method.h -o test.pch
~/llvm-project/clang/test/PCH# ~/llvm-project/build-rel/bin/clang++ -cc1 -x c++ cxx-method.cpp -include-pch test.pch -emit-llvm
~/llvm-project/clang/test/PCH# grep _Znwm cxx-method.ll
define dso_local nonnull i8* @_Znwm(i64 %size) #0 {
  %call = call noalias nonnull i8* @_Znwm(i64 %0) #2
  %call = call noalias nonnull i8* @_Znwm(i64 4) #3

Assuming I'm answering the correct question that the returns_nonnull is preserved through a PCH, the answer is yes.

[modimo]

@rsmith @lebedev.ri thoughts on adding this directly to FE generation? As mentioned this isn't strictly needed and the BE can elide the check but we can also not emit it to save on AST/IR processing cost.

[modimo]

Ping.

[bruno]

Assuming I'm answering the correct question that the returns_nonnull is preserved through a PCH, the answer is yes.

Thanks!

As mentioned this isn't strictly needed and the BE can elide the check but we can also not emit it to save on AST/IR processing cost.

It's a good way for clang based tooling to get the expected result if relying on shouldNullCheckAllocation for an accurate answer.

Maybe wait a few more days to see if anyone else has any comment, otherwise LGTM.

clang/lib/AST/ExprCXX.cpp

void *Mem =
       Ctx.Allocate(totalSizeToAlloc<Stmt *, SourceRange>(
                        IsArray + HasInit + NumPlacementArgs, IsParenTypeId),
                    alignof(CXXNewExpr));
   return new (Mem)
       CXXNewExpr(EmptyShell(), IsArray, NumPlacementArgs, IsParenTypeId);
 }
 
 bool CXXNewExpr::shouldNullCheckAllocation() const {
-  return getOperatorNew()
+  return !getOperatorNew()->hasAttr<ReturnsNonNullAttr>() &&
+         getOperatorNew()
              ->getType()
              ->castAs<FunctionProtoType>()
              ->isNothrow() &&
          !getOperatorNew()->isReservedGlobalPlacementOperator();
 }
 
 // CXXDeleteExpr
 QualType CXXDeleteExpr::getDestroyedType() const {

clang/test/CodeGenCXX/new.cpp

void t13(int n) {
   (void)new int[n]();
 
   // CHECK-NEXT: ret void
 }
 
 struct Alloc{
   int x;
   void* operator new[](size_t size);
+  __attribute__((returns_nonnull)) void *operator new[](size_t size, const std::nothrow_t &) throw();
   void operator delete[](void* p);
   ~Alloc();
 };
 
 void f() {
   // CHECK: call i8* @_ZN5AllocnaEm(i64 808)
   // CHECK: store i64 200
   // CHECK: call void @_ZN5AllocD1Ev(
   // CHECK: call void @_ZN5AllocdaEPv(i8*
   delete[] new Alloc[10][20];
+  // CHECK: [[P:%.*]] = call nonnull i8* @_ZN5AllocnaEmRKSt9nothrow_t(i64 808, {{.*}}) [[ATTR_NOUNWIND:#[^ ]*]]
+  // CHECK-NOT: icmp eq i8* [[P]], null
+  // CHECK: store i64 200
+  delete[] new (nothrow) Alloc[10][20];
   // CHECK: call noalias nonnull i8* @_Znwm
   // CHECK: call void @_ZdlPv(i8*
   delete new bool;
   // CHECK: ret void
 }
 
 namespace test15 {
   struct A { A(); ~A(); };
void f() {
     // CHECK: call void @_ZdlPv({{.*}}) [[ATTR_BUILTIN_DELETE:#[^ ]*]]
     delete p;
 
     // CHECK: call noalias nonnull i8* @_Znam(i64 12) [[ATTR_BUILTIN_NEW]]
     int *q = new int[3];
     // CHECK: call void @_ZdaPv({{.*}}) [[ATTR_BUILTIN_DELETE]]
     delete[] p; // expected-warning {{'delete[]' applied to a pointer that was allocated with 'new'; did you mean 'delete'?}}
 
-    // CHECK: call noalias i8* @_ZnamRKSt9nothrow_t(i64 3, {{.*}}) [[ATTR_BUILTIN_NOTHROW_NEW:#[^ ]*]]
+    // CHECK: call noalias i8* @_ZnamRKSt9nothrow_t(i64 3, {{.*}}) [[ATTR_NOBUILTIN_NOUNWIND_ALLOCSIZE:#[^ ]*]]
     (void) new (nothrow) S[3];
 
     // CHECK: call i8* @_Znwm15MyPlacementType(i64 4){{$}}
     (void) new (mpt) int;
   }
 
   // CHECK: declare i8* @_ZnamRKSt9nothrow_t(i64, {{.*}}) [[ATTR_NOBUILTIN_NOUNWIND_ALLOCSIZE:#[^ ]*]]