[OpenCL] Forbid size dependent types used as kernel arguments

[asavonic]

Summary

Size_t, intptr_t, uintptr_t and ptrdiff_t cannot be used as kernel
arguments, according to OpenCL Specification s6.9k:

The size in bytes of these types are implementation-defined and in
addition can also be different for the OpenCL device and the host
processor making it difficult to allocate buffer objects to be passed
as arguments to a kernel declared as pointer to these types.

[yaxunl]

This patch also adds check for array of structs. Can you include this in title or split to a separate patch?

lib/Sema/SemaDecl.cpp

8065 ↗

(On Diff #157005)

Can we record the real size_t/intptr_t/etc typedef and later on emit a note for it? It helps user to locate the culprit typedef in case of multiple typedefs.

8189 ↗

(On Diff #157005)

Can we have a test for this change? e.g. an array of structs

[Anastasia]

lib/Sema/SemaDecl.cpp

8186 ↗

(On Diff #157005)

I am a bit confused about this comment, do you mean a PointerType to a RecordType or an ArrayType of a RecordType?

8186 ↗

(On Diff #157005)

Also is there any test case covering this change?

8189 ↗

(On Diff #157005)

I am wondering if PT->getPointeeOrArrayElementType() is nullptr? Do we need to add an extra check?

[asavonic]

Added a diagnostic note for typedefs; moved unrelated changes to
https://reviews.llvm.org/D49723.

[asavonic]

In D49725#1173321, @yaxunl wrote:

This patch also adds check for array of structs. Can you include this in title or split to a separate patch?

I'm sorry, this change with arrays should actually go into https://reviews.llvm.org/D49723. Moved it there.

lib/Sema/SemaDecl.cpp

8065 ↗

(On Diff #157005)

I changed the patch to emit a note for all typedefs involved in
InvalidKernelParam. This way it works not only for size_t types, but
also for other invalid types which were typedef'ed.

8186 ↗

(On Diff #157005)

ArrayType of a RecordType. Fixed.

8186 ↗

(On Diff #157005)

I added 2 tests for this change in https://reviews.llvm.org/D49723.

8189 ↗

(On Diff #157005)

Added in https://reviews.llvm.org/D49723.

8189 ↗

(On Diff #157005)

It should not return null, unless the PT is null:
https://clang.llvm.org/doxygen/Type_8h_source.html#l06487

[Anastasia]

lib/Sema/SemaDecl.cpp

8267 ↗

(On Diff #157222)

Should this bit go into https://reviews.llvm.org/D49725?

[asavonic]

Moved unrelated change to https://reviews.llvm.org/D49723.

[asavonic]

lib/Sema/SemaDecl.cpp

8267 ↗

(On Diff #157222)

Right, thank you!

[yaxunl]

LGTM. Thanks!

[Anastasia]

LGTM! Thanks!

[aaron.ballman]

FYI: @asavonic, the email address you have associated with your commit id is AlexeySotkin@/etc/mailname which is getting stuck in the moderation queue as not being signed up to the mailing list. You may want to correct your svn information as I am not certain what our list software will think of that domain.

[AlexeySotkin]

In D49725#1183316, @aaron.ballman wrote:

FYI: @asavonic, the email address you have associated with your commit id is AlexeySotkin@/etc/mailname which is getting stuck in the moderation queue as not being signed up to the mailing list. You may want to correct your svn information as I am not certain what our list software will think of that domain.

Hi @aaron.ballman. It seems that this issue has broken mirroring with github repo. How I can fix it? Thanks.

cfe/trunk/lib/Sema/SemaDecl.cpp

enum OpenCLParamType {
   ValidKernelParam,
   PtrPtrKernelParam,
   PtrKernelParam,
   InvalidAddrSpacePtrKernelParam,
   InvalidKernelParam,
   RecordKernelParam
 };
 
+static bool isOpenCLSizeDependentType(ASTContext &C, QualType Ty) {
+  // Size dependent types are just typedefs to normal integer types
+  // (e.g. unsigned long), so we cannot distinguish them from other typedefs to
+  // integers other than by their names.
+  StringRef SizeTypeNames[] = {"size_t", "intptr_t", "uintptr_t", "ptrdiff_t"};
+
+  // Remove typedefs one by one until we reach a typedef
+  // for a size dependent type.
+  QualType DesugaredTy = Ty;
+  do {
+    ArrayRef<StringRef> Names(SizeTypeNames);
+    auto Match =
+        std::find(Names.begin(), Names.end(), DesugaredTy.getAsString());
+    if (Names.end() != Match)
+      return true;
+
+    Ty = DesugaredTy;
+    DesugaredTy = Ty.getSingleStepDesugaredType(C);
+  } while (DesugaredTy != Ty);
+
+  return false;
+}
+
 static OpenCLParamType getOpenCLKernelParameterType(Sema &S, QualType PT) {
   if (PT->isPointerType()) {
     QualType PointeeType = PT->getPointeeType();
     if (PointeeType->isPointerType())
       return PtrPtrKernelParam;
     if (PointeeType.getAddressSpace() == LangAS::opencl_generic ||
         PointeeType.getAddressSpace() == LangAS::opencl_private ||
         PointeeType.getAddressSpace() == LangAS::Default)
       return InvalidAddrSpacePtrKernelParam;
     return PtrKernelParam;
   }
 
-  // TODO: Forbid the other integer types (size_t, ptrdiff_t...) when they can
-  // be used as builtin types.
+  // OpenCL v1.2 s6.9.k:
+  // Arguments to kernel functions in a program cannot be declared with the
+  // built-in scalar types bool, half, size_t, ptrdiff_t, intptr_t, and
+  // uintptr_t or a struct and/or union that contain fields declared to be one
+  // of these built-in scalar types.
+  if (isOpenCLSizeDependentType(S.getASTContext(), PT))
+    return InvalidKernelParam;
 
   if (PT->isImageType())
     return PtrKernelParam;
 
   if (PT->isBooleanType() || PT->isEventT() || PT->isReserveIDT())
     return InvalidKernelParam;
 
   // OpenCL extension spec v1.2 s9.5:
case InvalidAddrSpacePtrKernelParam:
     // one of these built-in scalar types.
 
   case InvalidKernelParam:
     // OpenCL v1.2 s6.8 n:
     // A kernel function argument cannot be declared
     // of event_t type.
     // Do not diagnose half type since it is diagnosed as invalid argument
     // type for any function elsewhere.
-    if (!PT->isHalfType())
+    if (!PT->isHalfType()) {
       S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
+
+      // Explain what typedefs are involved.
+      const TypedefType *Typedef = nullptr;
+      while ((Typedef = PT->getAs<TypedefType>())) {
+        SourceLocation Loc = Typedef->getDecl()->getLocation();
+        // SourceLocation may be invalid for a built-in type.
+        if (Loc.isValid())
+          S.Diag(Loc, diag::note_entity_declared_at) << PT;
+        PT = Typedef->desugar();
+      }
+    }
+
     D.setInvalidType();
     return;
 
   case PtrKernelParam:
   case ValidKernelParam:
     ValidTypes.insert(PT.getTypePtr());
     return;
 

cfe/trunk/test/SemaOpenCL/invalid-kernel-parameters.cl

 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
 
 kernel void half_arg(half x) { } // expected-error{{declaring function parameter of type 'half' is not allowed; did you forget * ?}}
 
 #pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 
 // Disallowed: parameters with type
 // bool, half, size_t, ptrdiff_t, intptr_t, and uintptr_t
 // or a struct / union with any of these types in them
 
-// TODO: Ban int types, size_t, ptrdiff_t ...
+typedef __SIZE_TYPE__ size_t; // expected-note{{'size_t' (aka 'unsigned int') declared here}}
+                              // expected-note@-1{{'size_t' (aka 'unsigned int') declared here}}
+typedef __PTRDIFF_TYPE__ ptrdiff_t; // expected-note{{'ptrdiff_t' (aka 'int') declared here}}
+typedef __INTPTR_TYPE__ intptr_t; // expected-note{{'intptr_t' (aka 'int') declared here}}
+typedef __UINTPTR_TYPE__ uintptr_t; // expected-note{{'uintptr_t' (aka 'unsigned int') declared here}}
+
+kernel void size_t_arg(size_t x) {} // expected-error{{'size_t' (aka 'unsigned int') cannot be used as the type of a kernel parameter}}
+
+kernel void ptrdiff_t_arg(ptrdiff_t x) {} // expected-error{{'ptrdiff_t' (aka 'int') cannot be used as the type of a kernel parameter}}
+
+kernel void intptr_t_arg(intptr_t x) {} // expected-error{{'intptr_t' (aka 'int') cannot be used as the type of a kernel parameter}}
+
+kernel void uintptr_t_arg(uintptr_t x) {} // expected-error{{'uintptr_t' (aka 'unsigned int') cannot be used as the type of a kernel parameter}}
+
+typedef size_t size_ty;
+struct SizeTStruct { // expected-note{{within field of type 'SizeTStruct' declared here}}
+  size_ty s; // expected-note{{field of illegal type 'size_ty' (aka 'unsigned int') declared here}}
+};
+kernel void size_t_struct_arg(struct SizeTStruct x) {} // expected-error{{'struct SizeTStruct' cannot be used as the type of a kernel parameter}}
+
+union SizeTUnion { // expected-note{{within field of type 'SizeTUnion' declared here}}
+  size_t s; // expected-note{{field of illegal type 'size_t' (aka 'unsigned int') declared here}}
+  float f;
+};
+kernel void size_t_union_arg(union SizeTUnion x) {} // expected-error{{'union SizeTUnion' cannot be used as the type of a kernel parameter}}
+
+typedef size_t s_ty; // expected-note{{'s_ty' (aka 'unsigned int') declared here}}
+typedef s_ty ss_ty; // expected-note{{'ss_ty' (aka 'unsigned int') declared here}}
+kernel void typedef_to_size_t(ss_ty s) {} // expected-error{{'ss_ty' (aka 'unsigned int') cannot be used as the type of a kernel parameter}}
 
 kernel void bool_arg(bool x) { } // expected-error{{'bool' cannot be used as the type of a kernel parameter}}
 
 // half kernel argument is allowed when cl_khr_fp16 is enabled.
 kernel void half_arg(half x) { }
 
 typedef struct ContainsBool // expected-note{{within field of type 'ContainsBool' declared here}}
 {