[OpaquePtr] Verify Opaque pointer in function parameter

[zequanwu]

Summary

Verifying opaque pointer as function parameter when using with byval, byref,
inalloca, preallocated.

[aeubanks]

can you add some negative tests for this? e.g. make sure you see the proper message for something like

call void @f(ptr inalloca(%somenotsizedtype) %a)

there are some negative verifier tests in llvm/test/Verifier, search RUN: not opt

llvm/lib/IR/Verifier.cpp

1869

we still need a Visited in the new code to account for recursive types

we can delete the checks below for isSized() and always use your new code, then only check that the pointee type and inalloca/etc type match for non-opaque pointers

[zequanwu]

Update.

[aeubanks]

llvm/lib/IR/Verifier.cpp

1869

we still should keep this check that for non-opaque pointers, the element type matches the attribute type, I only meant to make the check for sized common between opaque and non-opaque pointers (and we should keep all the tests that were deleted)

[zequanwu]

Update.

[aeubanks]

lgtm

llvm/lib/IR/Verifier.cpp

void Verifier::verifyParameterAttrs(AttributeSet Attrs, Type *Ty,
 
   AttrBuilder IncompatibleAttrs = AttributeFuncs::typeIncompatible(Ty);
   Assert(!AttrBuilder(Attrs).overlaps(IncompatibleAttrs),
          "Wrong types for attribute: " +
              AttributeSet::get(Context, IncompatibleAttrs).getAsString(),
          V);
 
   if (PointerType *PTy = dyn_cast<PointerType>(Ty)) {
-    SmallPtrSet<Type*, 4> Visited;
-    if (!PTy->getElementType()->isSized(&Visited)) {
-      Assert(!Attrs.hasAttribute(Attribute::ByVal) &&
-             !Attrs.hasAttribute(Attribute::ByRef) &&
-             !Attrs.hasAttribute(Attribute::InAlloca) &&
-             !Attrs.hasAttribute(Attribute::Preallocated),
-             "Attributes 'byval', 'byref', 'inalloca', and 'preallocated' do not "
-             "support unsized types!",
-             V);
+    if (Attrs.hasAttribute(Attribute::ByVal)) {
+      SmallPtrSet<Type *, 4> Visited;
+      Assert(Attrs.getByValType()->isSized(&Visited),
+             "Attribute 'byval' does not support unsized types!", V);
     }
-    if (!isa<PointerType>(PTy->getElementType()))
-      Assert(!Attrs.hasAttribute(Attribute::SwiftError),
-             "Attribute 'swifterror' only applies to parameters "
-             "with pointer to pointer type!",
-             V);
-
     if (Attrs.hasAttribute(Attribute::ByRef)) {
-      Assert(Attrs.getByRefType() == PTy->getElementType(),
-             "Attribute 'byref' type does not match parameter!", V);
+      SmallPtrSet<Type *, 4> Visited;
+      Assert(Attrs.getByRefType()->isSized(&Visited),
+             "Attribute 'byref' does not support unsized types!", V);
     }
-
-    if (Attrs.hasAttribute(Attribute::ByVal) && Attrs.getByValType()) {
-      Assert(Attrs.getByValType() == PTy->getElementType(),
-             "Attribute 'byval' type does not match parameter!", V);
+    if (Attrs.hasAttribute(Attribute::InAlloca)) {
+      SmallPtrSet<Type *, 4> Visited;
+      Assert(Attrs.getInAllocaType()->isSized(&Visited),
+             "Attribute 'inalloca' does not support unsized types!", V);
     }
-
     if (Attrs.hasAttribute(Attribute::Preallocated)) {
-      Assert(Attrs.getPreallocatedType() == PTy->getElementType(),
-             "Attribute 'preallocated' type does not match parameter!", V);
-    }
-
-    if (Attrs.hasAttribute(Attribute::InAlloca)) {
-      Assert(Attrs.getInAllocaType() == PTy->getElementType(),
-             "Attribute 'inalloca' type does not match parameter!", V);
+      SmallPtrSet<Type *, 4> Visited;
+      Assert(Attrs.getPreallocatedType()->isSized(&Visited),
+             "Attribute 'preallocated' does not support unsized types!", V);
     }
+    if (!PTy->isOpaque() && !isa<PointerType>(PTy->getElementType()))
+      Assert(!Attrs.hasAttribute(Attribute::SwiftError),
+             "Attribute 'swifterror' only applies to parameters "
+             "with pointer to pointer type!",
+             V);
   } else {
     Assert(!Attrs.hasAttribute(Attribute::ByVal),
            "Attribute 'byval' only applies to parameters with pointer type!",
            V);
     Assert(!Attrs.hasAttribute(Attribute::ByRef),
            "Attribute 'byref' only applies to parameters with pointer type!",
            V);
     Assert(!Attrs.hasAttribute(Attribute::SwiftError),
            "Attribute 'swifterror' only applies to parameters "
            "with pointer type!",
            V);

llvm/test/Assembler/invalid-byval-type1.ll

-; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
-
-; CHECK: Attribute 'byval' type does not match parameter!
-declare void @foo(i32* byval(i8))

llvm/test/Assembler/invalid-byval-type3.ll

 ; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
 
-; CHECK: Attributes 'byval'{{.*}} do not support unsized types!
+; CHECK: Attribute 'byval' does not support unsized types!
 declare void @foo(void()* byval(void()))

llvm/test/Verifier/byref.ll

 ; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
 
-; CHECK: Attribute 'byref' type does not match parameter!
-; CHECK-NEXT: void (i32*)* @byref_mismatched_pointee_type0
-define void @byref_mismatched_pointee_type0(i32* byref(i8)) {
-  ret void
-}
-
-; CHECK: Attribute 'byref' type does not match parameter!
-; CHECK-NEXT: void (i8*)* @byref_mismatched_pointee_type1
-define void @byref_mismatched_pointee_type1(i8* byref(i32)) {
-  ret void
-}
-
 %opaque.ty = type opaque
 
-; CHECK: Attributes 'byval', 'byref', 'inalloca', and 'preallocated' do not support unsized types!
+; CHECK: Attribute 'byref' does not support unsized types!
 ; CHECK-NEXT: void (%opaque.ty*)* @byref_unsized
 define void @byref_unsized(%opaque.ty* byref(%opaque.ty)) {
   ret void
 }
 
 ; CHECK: Attributes 'byval', 'inalloca', 'preallocated', 'inreg', 'nest', 'byref', and 'sret' are incompatible!
 ; CHECK-NEXT: void (i32*)* @byref_byval
 define void @byref_byval(i32* byref(i32) byval(i32)) {

llvm/test/Verifier/inalloca1.ll

 
 declare void @d(i64* nest inalloca(i64) %p)
 ; CHECK: Attributes {{.*}} are incompatible
 
 declare void @e(i64* readonly inalloca(i64) %p)
 ; CHECK: Attributes {{.*}} are incompatible
 
 declare void @f(void ()* inalloca(void()) %p)
-; CHECK: do not support unsized types
+; CHECK: Attribute 'inalloca' does not support unsized types
 
 declare void @g(i32* inalloca(i32) %p, i32 %p2)
 ; CHECK: inalloca isn't on the last parameter!
 
-; CHECK: Attribute 'inalloca' type does not match parameter!
-; CHECK-NEXT: void (i32*)* @inalloca_mismatched_pointee_type0
-define void @inalloca_mismatched_pointee_type0(i32* inalloca(i8)) {
-  ret void
-}
-
 ; CHECK: Wrong types for attribute:
 ; CHECK-NEXT: void (i8)* @inalloca_not_pointer
 define void @inalloca_not_pointer(i8 byref(i8)) {
   ret void
 }

llvm/test/Verifier/opaque-ptr-invalid.ll

+; RUN: not opt -verify < %s 2>&1 | FileCheck %s
+
+; CHECK: Attribute 'inalloca' does not support unsized types!
+; CHECK-NEXT: void (ptr)* @f
+define void @f(ptr inalloca(token)) {
+    ret void
+}

llvm/test/Verifier/preallocated-invalid.ll

 
 ; CHECK: must be between 0 and corresponding
 define void @preallocated_setup_arg_index_in_bounds() {
     %cs = call token @llvm.call.preallocated.setup(i32 2)
     %a0 = call i8* @llvm.call.preallocated.arg(token %cs, i32 2) preallocated(i32)
     ret void
 }
 
-; CHECK: Attribute 'preallocated' type does not match parameter
-define void @preallocated_attribute_type_mismatch() {
-    %cs = call token @llvm.call.preallocated.setup(i32 1)
-    %x = call i8* @llvm.call.preallocated.arg(token %cs, i32 0) preallocated(i32)
-    %y = bitcast i8* %x to i32*
-    call void @foo1(i32* preallocated(i8) %y) ["preallocated"(token %cs)]
-    ret void
-}
-
 ; CHECK: preallocated operand either requires a preallocated bundle or the call to be musttail
 define void @preallocated_require_bundle() {
     %cs = call token @llvm.call.preallocated.setup(i32 1)
     %x = call i8* @llvm.call.preallocated.arg(token %cs, i32 0) preallocated(i32)
     %y = bitcast i8* %x to i32*
     call void @foo1(i32* preallocated(i32) %y)
     ret void
 }