Page MenuHomePhabricator

[SYCL] Diagnose uses of zero length arrays
ClosedPublic

Authored by Fznamznon on Nov 17 2021, 5:01 AM.

Details

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.

Diff Detail

Event Timeline

Fznamznon created this revision.Nov 17 2021, 5:01 AM
Fznamznon requested review of this revision.Nov 17 2021, 5:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 5:01 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Fznamznon added a comment.EditedNov 17 2021, 5:11 AM

@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.

Why the need for this restriction ?

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 ?

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

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.

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.

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 edited the summary of this revision. (Show Details)Nov 17 2021, 6:20 AM

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 added a comment.Nov 25 2021, 1:01 AM

LGTM, with a couple of minor suggestions.

clang/lib/Sema/SemaSYCL.cpp
68–75
125
Fznamznon updated this revision to Diff 390346.Nov 29 2021, 7:00 AM
Fznamznon marked an inline comment as done.

Apply suggestion from @bader

Fznamznon marked an inline comment as not done.Nov 29 2021, 7:04 AM
Fznamznon added inline comments.
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?

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

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 updated this revision to Diff 390376.Nov 29 2021, 8:21 AM

An attempt to fix review page

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.

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 updated this revision to Diff 392080.Dec 6 2021, 8:27 AM

Applied suggestions from Aaron

Fznamznon marked 2 inline comments as done.Dec 6 2021, 8:28 AM
This revision is now accepted and ready to land.Dec 6 2021, 8:36 AM

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.

This revision was automatically updated to reflect the committed changes.