[CodeGen] -fno-delete-null-pointer-checks: change dereferenceable to dereferenceable_or_null

[MaskRay]

Summary

After D17993, with -fno-delete-null-pointer-checks we add the dereferenceable attribute to the this pointer.

We have observed that one internal target which worked before fails even with -fno-delete-null-pointer-checks.
Switching to dereferenceable_or_null fixes the problem.

dereferenceable currently does not always respect NullPointerIsValid and may
imply nonnull and lead to aggressive optimization. The optimization may be
related to CallBase::isReturnNonNull, Argument::hasNonNullAttr, or
Value::getPointerDereferenceableBytes. See D66664 and D66618 for some discussions.

[rsmith]

This was discussed on the original patch (https://reviews.llvm.org/D17993#inline-830995) and @jdoerfert argued that dereferenceable is correct even in NullPointerIsValid mode. Does this indicate a bug in the middle-end? If so, we should add a FIXME here to remove the workaround once the underlying bug is fixed.

[bkramer]

While it would be nice for dereferenceable to not imply nonnull, the implementation currently assumes it does and will speculate loads based on that. See llvm::Value::getPointerDereferenceableBytes and its users.

[jdoerfert]

In D92297#2422568, @bkramer wrote:

While it would be nice for dereferenceable to not imply nonnull, the implementation currently assumes it does and will speculate loads based on that. See llvm::Value::getPointerDereferenceableBytes and its users.

LLVM-Core unfortunately conflated null as 0x000000 and as "invalid pointer"/"non-dereferencable". This patch just works around that. While this is fine for now, a FIXME should be placed and LLVM-Core needs fixing.

I briefly scanned my history and found D66664 which is relevant and gives some context and my attempt to fix this last year.

[MaskRay]

Add FIXME

[MaskRay]

clang/lib/CodeGen/CGCall.cpp

2176

@rsmith @jdoerfert Am I right about the FIXME issue here?

Should I link to D66618 instead?

[rsmith]

Looks good. There are a few other places where we unconditionally insert dereferenceable attributes (for reference parameters / return types, and for C99's T [static] array parameters). We should presumably apply the same workaround everywhere we add dereferenceable.

clang/lib/CodeGen/CGCall.cpp

2178

[MaskRay]

Fix a comment

clang/lib/CodeGen/CGCall.cpp

if (FI.isInstanceMethod() && !IRFunctionArgs.hasInallocaArg() &&
 
     assert(IRArgs.second == 1 && "Expected only a single `this` pointer.");
 
     llvm::AttrBuilder Attrs;
 
     if (!CodeGenOpts.NullPointerIsValid &&
         getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
       Attrs.addAttribute(llvm::Attribute::NonNull);
-    }
-
       Attrs.addDereferenceableAttr(
           getMinimumObjectSize(
               FI.arg_begin()->type.castAs<PointerType>()->getPointeeType())
               .getQuantity());
+    } else {
+      // FIXME dereferenceable should be correct here, regardless of
+      // NullPointerIsValid. However, dereferenceable currently does not always
+      // respect NullPointerIsValid and may imply nonnull and break the program.
+      // See https://reviews.llvm.org/D66618 for discussions.
+      Attrs.addDereferenceableOrNullAttr(
+          getMinimumObjectSize(
+              FI.arg_begin()->type.castAs<PointerType>()->getPointeeType())
+              .getQuantity());
+    }
 
     ArgAttrs[IRArgs.first] = llvm::AttributeSet::get(getLLVMContext(), Attrs);
   }
 
   unsigned ArgNo = 0;
   for (CGFunctionInfo::const_arg_iterator I = FI.arg_begin(),
                                           E = FI.arg_end();
        I != E; ++I, ++ArgNo) {

clang/test/CodeGenCXX/this-nonnull.cpp

 // RUN: %clang_cc1 -S -emit-llvm -o - -triple=x86_64-linux-gnu %s | FileCheck %s -check-prefix=CHECK-YES
 // RUN: %clang_cc1 -S -emit-llvm -o - -fno-delete-null-pointer-checks -triple=x86_64-linux-gnu %s | FileCheck %s -check-prefix=CHECK-NO
 
 struct Struct {
   int many;
   int member;
   int fields;
   void ReturnsVoid();
 };
 
 void TestReturnsVoid(Struct &s) {
   s.ReturnsVoid();
 
   // CHECK-YES: call void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* nonnull dereferenceable(12) %0)
-  // CHECK-NO: call void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* dereferenceable(12) %0)
+  /// FIXME Use dereferenceable after dereferenceable respects NullPointerIsValid.
+  // CHECK-NO: call void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* dereferenceable_or_null(12) %0)
 }
 
 // CHECK-YES: declare void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* nonnull dereferenceable(12))
-// CHECK-NO: declare void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* dereferenceable(12))
+// CHECK-NO: declare void @_ZN6Struct11ReturnsVoidEv(%struct.Struct* dereferenceable_or_null(12))