[SYCL] Diagnose uses of zero length arrays

[Fznamznon]

Summary

Adds diagnosing on attempt to use zero length arrays, pointers, refs, arrays
of them and structs/classes containing all of it.
In case a struct/class with zero length array is used this emits a set
of notes pointing out how zero length array got into used struct, like
this:

struct ContainsArr {
  int A[0]; // note: field of illegal type declared here
};
struct Wrapper {
  ContainsArr F; // note: within field of type ContainsArr declared here
  // ...
}

// Device code
Wrapper W;
W.use(); // error: zero-length arrays are not permitted

Total deep check of each used declaration may result in double
diagnosing at the same location.

[Fznamznon]

@jdoerfert , @yaxunl , please let me know if similar thing would be useful for OpenMP/HIP/CUDA as well. I can generalize it by using targetDiag.

[Naghasan]

Why the need for this restriction ?

[Naghasan]

right, TIL sizeof (ContainsArr) is 0 according to clang. I can see trouble arising.

@aaron.ballman maybe you know better, is that intended for the extension ?

[aaron.ballman]

In D114080#3137462, @Naghasan wrote:

right, TIL sizeof (ContainsArr) is 0 according to clang. I can see trouble arising.

@aaron.ballman maybe you know better, is that intended for the extension ?

I don't *know*, but I have two educated guesses that it is intended. GCC compatibility (GCC also has this behavior) and the relationship between zero-sized array members and flexible array members. Clang allows a flexible array member to be used in a structure with no members and it behaves the same as a zero-sized array in a structure with no members: https://godbolt.org/z/Gx7ozxYKz

[Fznamznon]

In D114080#3137430, @Naghasan wrote:

Why the need for this restriction ?

Zero length arrays are not supported by pure C++. It is an extension supported by clang. They are also not allowed in OpenCL and SPIR-V, and I even though I cannot grep an explicit restriction for them in SYCL 2020 spec (I guess that is just because it is an extension and not pure C++) It doesn't make sense to allow them here since memory allocation in device code is prohibited in SYCL by the spec. This diagnostic prevents further not always user friendly errors coming from SPIR-V emitters or from backends like OpenCL that explicitly disallow them on early stage and in user-friendly form. I believe other GPU programming models also may not allow them for the same reason - restriction on memory allocation.

[Naghasan]

In D114080#3137480, @aaron.ballman wrote:

In D114080#3137462, @Naghasan wrote:

right, TIL sizeof (ContainsArr) is 0 according to clang. I can see trouble arising.

@aaron.ballman maybe you know better, is that intended for the extension ?

I don't *know*, but I have two educated guesses that it is intended. GCC compatibility (GCC also has this behavior) and the relationship between zero-sized array members and flexible array members. Clang allows a flexible array member to be used in a structure with no members and it behaves the same as a zero-sized array in a structure with no members: https://godbolt.org/z/Gx7ozxYKz

I see. I was confused as it normally C++ requires at least 1 byte. But I guess you need that for compatibility with C.

In D114080#3137482, @Fznamznon wrote:

In D114080#3137430, @Naghasan wrote:

Why the need for this restriction ?

Zero length arrays are not supported by pure C++. It is an extension supported by clang. They are also not allowed in OpenCL and SPIR-V, and I even though I cannot grep an explicit restriction for them in SYCL 2020 spec (I guess that is just because it is an extension and not pure C++) It doesn't make sense to allow them here since memory allocation in device code is prohibited in SYCL by the spec. This diagnostic prevents further not always user friendly errors coming from SPIR-V emitters or from backends like OpenCL that explicitly disallow them on early stage and in user-friendly form. I believe other GPU programming models also may not allow them for the same reason - restriction on memory allocation.

SYCL won't mention this as it is an extension indeed. Same for VLA. SPIR-V is another question though, while 0 element array are not allowed, empty structs are (but IIRC size of an empty struct is not clear).

It doesn't make sense to allow them here since memory allocation in device code is prohibited in SYCL by the spec

I agree, but having a field defined is different from it being used. It is actually not uncommon to see SYCL functor passed with many unused fields (Eigen is good example).

[Fznamznon]

I agree, but having a field defined is different from it being used. It is actually not uncommon to see SYCL functor passed with many unused fields (Eigen is good example).

Are you saying that it is better to diagnose just used fields?
I believe it is safer to diagnose everything, because some backends or SPIR-V emitters may not expect that and crash or throw non-user friendly errors, because usually it is up to front-end to emit errors when some unsupported feature appears in device code (even unused).

[bader]

LGTM, with a couple of minor suggestions.

clang/lib/Sema/SemaSYCL.cpp

68–75

125

[Fznamznon]

Apply suggestion from @bader

[Fznamznon]

clang/lib/Sema/SemaSYCL.cpp

68–75

I'm not sure about this suggestion. I've made this place this way so It is easy to extend it to diagnose other types (for example variable length arrays). Is it ok if I leave it as is?

[erichkeane]

I note that this is missing a test, otherwise I don't see any issues with it from my end.

[Fznamznon]

In D114080#3158323, @erichkeane wrote:

I note that this is missing a test, otherwise I don't see any issues with it from my end.

There is a test SemaSYCL/zero-length-arrays.cpp. For some reason after I updated this revision Phabricator doesn't show the full diff, but it shows both commits. The first commit with main changes and the test is available here - https://reviews.llvm.org/D114080?id=387904 .

[Fznamznon]

An attempt to fix review page

[Fznamznon]

In D114080#3158368, @Fznamznon wrote:

In D114080#3158323, @erichkeane wrote:

I note that this is missing a test, otherwise I don't see any issues with it from my end.

There is a test SemaSYCL/zero-length-arrays.cpp. For some reason after I updated this revision Phabricator doesn't show the full diff, but it shows both commits. The first commit with main changes and the test is available here - https://reviews.llvm.org/D114080?id=387904 .

It seems I was able to fix that. Now the link to this revision shows the full diff.

[Fznamznon]

Ping.

[aaron.ballman]

Just a few minor nits from me.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11446–11447

We should reuse err_typecheck_zero_array_size and give it a %select for the reason the zero-length array is not permitted.

clang/lib/Sema/SemaSYCL.cpp

76–82

Can elide the braces here per the usual coding conventions.

[Fznamznon]

Applied suggestions from Aaron

[aaron.ballman]

LGTM

[Fznamznon]

Ping.

[Fznamznon]

I hope that someone not from Intel will take a look as well. If this won't happen for reasonable amount of time, I'll submit this patch.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_typecheck_invalid_restrict_not_pointer : Error<
   "restrict requires a pointer or reference (%0 is invalid)">;
 def err_typecheck_invalid_restrict_not_pointer_noarg : Error<
   "restrict requires a pointer or reference">;
 def err_typecheck_invalid_restrict_invalid_pointee : Error<
   "pointer to function type %0 may not be 'restrict' qualified">;
 def ext_typecheck_zero_array_size : Extension<
   "zero size arrays are an extension">, InGroup<ZeroLengthArray>;
 def err_typecheck_zero_array_size : Error<
-  "zero-length arrays are not permitted in C++">;
+  "zero-length arrays are not permitted in %select{C++|SYCL device code}0">;
 def err_array_size_non_int : Error<"size of array has non-integer type %0">;
 def err_init_element_not_constant : Error<
   "initializer element is not a compile-time constant">;
 def ext_aggregate_init_not_constant : Extension<
   "initializer for aggregate is not a compile-time constant">, InGroup<C99>;
 def err_local_cant_init : Error<
   "'__local' variable cannot have an initializer">;
 def err_loader_uninitialized_cant_init
def warn_sycl_kernel_invalid_template_param_type : Warning<
   "template parameter of a function template with the 'sycl_kernel' attribute"
   " cannot be a non-type template parameter">, InGroup<IgnoredAttributes>;
 def warn_sycl_kernel_num_of_function_params : Warning<
   "function template with 'sycl_kernel' attribute must have a single parameter">,
   InGroup<IgnoredAttributes>;
 def warn_sycl_kernel_return_type : Warning<
   "function template with 'sycl_kernel' attribute must have a 'void' return type">,
   InGroup<IgnoredAttributes>;
 
 def err_bit_int_bad_size : Error<"%select{signed|unsigned}0 _BitInt must "
                                  "have a bit size of at least %select{2|1}0">;
 def err_bit_int_max_size : Error<"%select{signed|unsigned}0 _BitInt of bit "
                                  "sizes greater than %1 not supported">;
 
 // errors of expect.with.probability
 def err_probability_not_constant_float : Error<
    "probability argument to __builtin_expect_with_probability must be constant "
    "floating-point expression">;

clang/include/clang/Sema/Sema.h

public:
   ///   it's never codegen'ed, creates a deferred diagnostic to be emitted if
   ///   and when the caller is codegen'ed, and returns true.
   ///
   /// - Otherwise, returns true without emitting any diagnostics.
   ///
   /// Adds Callee to DeviceCallGraph if we don't know if its caller will be
   /// codegen'ed yet.
   bool checkSYCLDeviceFunction(SourceLocation Loc, FunctionDecl *Callee);
+  void deepTypeCheckForSYCLDevice(SourceLocation UsedAt,
+                                  llvm::DenseSet<QualType> Visited,
+                                  ValueDecl *DeclToCheck);
 };
 
 /// RAII object that enters a new expression evaluation context.
 class EnterExpressionEvaluationContext {
   Sema &Actions;
   bool Entered = true;
 
 public:

clang/lib/Sema/Sema.cpp

Sema::SemaDiagnosticBuilder Sema::Diag(SourceLocation Loc, unsigned DiagID,
   SetIsLastErrorImmediate(DB.isImmediate());
   return DB;
 }
 
 void Sema::checkTypeSupport(QualType Ty, SourceLocation Loc, ValueDecl *D) {
   if (isUnevaluatedContext() || Ty.isNull())
     return;
 
+  // The original idea behind checkTypeSupport function is that unused
+  // declarations can be replaced with an array of bytes of the same size during
+  // codegen, such replacement doesn't seem to be possible for types without
+  // constant byte size like zero length arrays. So, do a deep check for SYCL.
+  if (D && LangOpts.SYCLIsDevice) {
+    llvm::DenseSet<QualType> Visited;
+    deepTypeCheckForSYCLDevice(Loc, Visited, D);
+  }
+
   Decl *C = cast<Decl>(getCurLexicalContext());
 
   // Memcpy operations for structs containing a member with unsupported type
   // are ok, though.
   if (const auto *MD = dyn_cast<CXXMethodDecl>(C)) {
     if ((MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) &&
         MD->isTrivial())
       return;

clang/lib/Sema/SemaSYCL.cpp

bool Sema::checkSYCLDeviceFunction(SourceLocation Loc, FunctionDecl *Callee) {
   if (isUnevaluatedContext() || isConstantEvaluated())
     return true;
 
   SemaDiagnosticBuilder::Kind DiagKind = SemaDiagnosticBuilder::K_Nop;
 
   return DiagKind != SemaDiagnosticBuilder::K_Immediate &&
          DiagKind != SemaDiagnosticBuilder::K_ImmediateWithCallStack;
 }
+
+static bool isZeroSizedArray(Sema &SemaRef, QualType Ty) {
+  if (const auto *CAT = SemaRef.getASTContext().getAsConstantArrayType(Ty))
+    return CAT->getSize() == 0;
+  return false;
+}
+
+void Sema::deepTypeCheckForSYCLDevice(SourceLocation UsedAt,
+                                      llvm::DenseSet<QualType> Visited,
+                                      ValueDecl *DeclToCheck) {
+  assert(getLangOpts().SYCLIsDevice &&
+         "Should only be called during SYCL compilation");
+  // Emit notes only for the first discovered declaration of unsupported type
+  // to avoid mess of notes. This flag is to track that error already happened.
+  bool NeedToEmitNotes = true;
+
+  auto Check = [&](QualType TypeToCheck, const ValueDecl *D) {
+    bool ErrorFound = false;
+    if (isZeroSizedArray(*this, TypeToCheck)) {
+      SYCLDiagIfDeviceCode(UsedAt, diag::err_typecheck_zero_array_size) << 1;
+      ErrorFound = true;
+    }
+    // Checks for other types can also be done here.
+    if (ErrorFound) {
+      if (NeedToEmitNotes) {
+        if (auto *FD = dyn_cast<FieldDecl>(D))
+          SYCLDiagIfDeviceCode(FD->getLocation(),
+                               diag::note_illegal_field_declared_here)
+              << FD->getType()->isPointerType() << FD->getType();
+        else
+          SYCLDiagIfDeviceCode(D->getLocation(), diag::note_declared_at);
+      }
+    }
+
+    return ErrorFound;
+  };
+
+  // In case we have a Record used do the DFS for a bad field.
+  SmallVector<const ValueDecl *, 4> StackForRecursion;
+  StackForRecursion.push_back(DeclToCheck);
+
+  // While doing DFS save how we get there to emit a nice set of notes.
+  SmallVector<const FieldDecl *, 4> History;
+  History.push_back(nullptr);
+
+  do {
+    const ValueDecl *Next = StackForRecursion.pop_back_val();
+    if (!Next) {
+      assert(!History.empty());
+      // Found a marker, we have gone up a level.
+      History.pop_back();
+      continue;
+    }
+    QualType NextTy = Next->getType();
+
+    if (!Visited.insert(NextTy).second)
+      continue;
+
+    auto EmitHistory = [&]() {
+      // The first element is always nullptr.
+      for (uint64_t Index = 1; Index < History.size(); ++Index) {
+        SYCLDiagIfDeviceCode(History[Index]->getLocation(),
+                             diag::note_within_field_of_type)
+            << History[Index]->getType();
+      }
+    };
+
+    if (Check(NextTy, Next)) {
+      if (NeedToEmitNotes)
+        EmitHistory();
+      NeedToEmitNotes = false;
+    }
+
+    // In case pointer/array/reference type is met get pointee type, then
+    // proceed with that type.
+    while (NextTy->isAnyPointerType() || NextTy->isArrayType() ||
+           NextTy->isReferenceType()) {
+      if (NextTy->isArrayType())
+        NextTy = QualType{NextTy->getArrayElementTypeNoTypeQual(), 0};
+      else
+        NextTy = NextTy->getPointeeType();
+      if (Check(NextTy, Next)) {
+        if (NeedToEmitNotes)
+          EmitHistory();
+        NeedToEmitNotes = false;
+      }
+    }
+
+    if (const auto *RecDecl = NextTy->getAsRecordDecl()) {
+      if (auto *NextFD = dyn_cast<FieldDecl>(Next))
+        History.push_back(NextFD);
+      // When nullptr is discovered, this means we've gone back up a level, so
+      // the history should be cleaned.
+      StackForRecursion.push_back(nullptr);
+      llvm::copy(RecDecl->fields(), std::back_inserter(StackForRecursion));
+    }
+  } while (!StackForRecursion.empty());
+}

clang/lib/Sema/SemaType.cpp

if (!R.isUsable()) {
         return QualType();
       }
       if (ConstVal == 0) {
         // GCC accepts zero sized static arrays. We allow them when
         // we're not in a SFINAE context.
         Diag(ArraySize->getBeginLoc(),
              isSFINAEContext() ? diag::err_typecheck_zero_array_size
                                : diag::ext_typecheck_zero_array_size)
-            << ArraySize->getSourceRange();
+            << 0 << ArraySize->getSourceRange();
       }
 
       // Is the array too large?
       unsigned ActiveSizeBits =
           (!T->isDependentType() && !T->isVariablyModifiedType() &&
            !T->isIncompleteType() && !T->isUndeducedType())
               ? ConstantArrayType::getNumAddressingBits(Context, T, ConstVal)
               : ConstVal.getActiveBits();

clang/test/SemaSYCL/zero-length-arrays.cpp

+// RUN: %clang_cc1 -fsycl-is-device -triple spir64 -fsyntax-only -verify %s
+//
+// This test checks if compiler reports compilation error on an attempt to use
+// a zero-length array inside device code.
+
+template <typename Name, typename Func>
+__attribute__((sycl_kernel)) void kernel(const Func &kernelFunc) {
+  // expected-note@+1 5{{called by 'kernel}}
+  kernelFunc(); // #KernelObjCall
+}
+
+typedef float ZEROARR[0];
+
+struct Wrapper {
+  int A;
+  int BadArray[0]; // expected-note 3{{field of illegal type 'int[0]' declared here}}
+};
+
+struct WrapperOfWrapper { // expected-error 2{{zero-length arrays are not permitted in SYCL device code}}
+  Wrapper F;              // expected-note 2{{within field of type 'Wrapper' declared here}}
+  ZEROARR *Ptr;           //expected-note 5{{field of illegal pointer type 'ZEROARR *' (aka 'float (*)[0]') declared here}}
+};
+
+template <unsigned Size> struct InnerTemplated {
+  double Array[Size]; // expected-note 8{{field of illegal type 'double[0]' declared here}}
+};
+
+template <unsigned Size, typename Ty> struct Templated {
+  unsigned A;
+  Ty Arr[Size];
+  InnerTemplated<Size> Array[Size + 1]; // expected-note 8{{within field of type 'InnerTemplated<0U>[1]' declared here}}
+};
+
+struct KernelSt {
+  int A;
+  int BadArray[0]; // expected-note {{field of illegal type 'int[0]' declared here}}
+  void operator()() const {}
+};
+
+WrapperOfWrapper offendingFoo() {
+  // expected-note@+1 {{called by 'offendingFoo'}}
+  return WrapperOfWrapper{};
+}
+
+template <unsigned Size>
+void templatedContext() {
+  Templated<Size, float> Var;
+  // expected-error@#KernelObjCall 2{{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@#KernelObjCall {{called by 'kernel<TempContext, (lambda at}}
+  // expected-note@+1 {{in instantiation of function template specialization}}
+  kernel<class TempContext>([=] {
+    // expected-note@+1 {{within field of type 'Templated<0U, float>' declared here}}
+    (void)Var; // expected-error 2{{zero-length arrays are not permitted in SYCL device code}}
+  });
+  // expected-error@#KernelObjCall {{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@+2 {{in instantiation of function template specialization}}
+  // expected-note@+1 {{within field of type 'Templated<0U, float>' declared here}}
+  kernel<class TempContext1>([Var] {
+  });
+}
+
+void foo(const unsigned X) {
+  int Arr[0];      // expected-note 2{{declared here}}
+  ZEROARR TypeDef; // expected-note {{declared here}}
+  ZEROARR *Ptr;    // expected-note {{declared here}}
+                   // expected-error@#KernelObjCall 3{{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@+1 {{in instantiation of function template specialization}}
+  kernel<class Simple>([=]() {
+    (void)Arr;     // expected-error {{zero-length arrays are not permitted in SYCL device code}}
+    (void)TypeDef; // expected-error {{zero-length arrays are not permitted in SYCL device code}}
+    // expected-note@+1 {{field of illegal pointer type 'ZEROARR *' (aka 'float (*)[0]') declared here}}
+    (void)Ptr; // expected-error {{zero-length arrays are not permitted in SYCL device code}}
+  });
+  // expected-error@#KernelObjCall {{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@+2 {{in instantiation of function template specialization}}
+  // expected-note@+1 {{field of illegal type 'int[0]' declared here}}
+  kernel<class Simple1>([Arr] { // expected-error {{zero-length arrays are not permitted in SYCL device code}}
+  });
+  WrapperOfWrapper St;
+  // expected-error@#KernelObjCall 2{{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@+1 {{in instantiation of function template specialization}}
+  kernel<class SimpleStruct>([=] {
+    // expected-note@+1 {{within field of type 'WrapperOfWrapper' declared here}}
+    (void)St.F.BadArray; // expected-error 4{{zero-length arrays are not permitted in SYCL device code}}
+  });
+  // expected-error@#KernelObjCall 2{{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@+2 {{in instantiation of function template specialization}}
+  // expected-note@+1 {{within field of type 'WrapperOfWrapper' declared here}}
+  kernel<class SimpleStruct1>([St] { // expected-error 2{{zero-length arrays are not permitted in SYCL device code}}
+  });
+
+  Templated<1, int> OK;
+  Templated<1 - 1, double> Weirdo;
+  Templated<0, float> Zero;
+  // expected-error@#KernelObjCall 4{{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@+1 {{in instantiation of function template specialization}}
+  kernel<class UseTemplated>([=] {
+    (void)OK;   // No errors expected
+    (void)Zero; // expected-error 2{{zero-length arrays are not permitted in SYCL device code}}
+    // expected-note@+1 {{within field of type 'Templated<1 - 1, double>' declared here}}
+    int A = Weirdo.A; // expected-error 2{{zero-length arrays are not permitted in SYCL device code}}
+  });
+
+  // expected-note@#KernelObjCall {{called by 'kernel<UseTemplated1, (lambda at}}
+  // expected-error@#KernelObjCall 2{{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@+2 {{in instantiation of function template specialization}}
+  // expected-note@+1 {{within field of type 'Templated<0, float>' declared here}}
+  kernel<class UseTemplated1>([Zero] { // expected-error 2{{zero-length arrays are not permitted in SYCL device code}}
+  });
+
+  templatedContext<10>();
+  // expected-note@+1 2{{in instantiation of function template specialization}}
+  templatedContext<0>();
+
+  KernelSt K;
+  // expected-error@#KernelObjCall {{zero-length arrays are not permitted in SYCL device code}}
+  // expected-note@+1 {{in instantiation of function template specialization}}
+  kernel<class UseFunctor>(K);
+
+  // expected-note@#KernelObjCall {{called by 'kernel<ReturnFromFunc, (lambda at}}
+  kernel<class ReturnFromFunc>([=] {
+    // expected-note@+1 {{called by 'operator()'}}
+    offendingFoo();
+  });
+}