[PATCH] D49725: [OpenCL] Forbid size dependent types used as kernel arguments

[Andrew Savonichev via Phabricator via cfe-commits]

asavonic marked 5 inline comments as done.
asavonic added a comment.

In https://reviews.llvm.org/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.



================
Comment at: lib/Sema/SemaDecl.cpp:8065
+        std::find(Names.begin(), Names.end(), DesugaredTy.getAsString());
+    if (Names.end() != Match)
+      return true;
----------------
yaxunl wrote:
> 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.
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.



================
Comment at: lib/Sema/SemaDecl.cpp:8186
 
-  const RecordDecl *PD = PT->castAs<RecordType>()->getDecl();
-  VisitStack.push_back(PD);
+  // At this point we already handled everything except of a RecordType or
+  // an ArrayType[RecordType].
----------------
Anastasia wrote:
> Anastasia wrote:
> > I am a bit confused about this comment, `do you mean a PointerType to a RecordType or an ArrayType of a RecordType`?
> Also is there any test case covering this change?
ArrayType of a RecordType. Fixed.


================
Comment at: lib/Sema/SemaDecl.cpp:8186
 
-  const RecordDecl *PD = PT->castAs<RecordType>()->getDecl();
-  VisitStack.push_back(PD);
+  // At this point we already handled everything except of a RecordType or
+  // an ArrayType[RecordType].
----------------
asavonic wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > I am a bit confused about this comment, `do you mean a PointerType to a RecordType or an ArrayType of a RecordType`?
> > Also is there any test case covering this change?
> ArrayType of a RecordType. Fixed.
I added 2 tests for this change in https://reviews.llvm.org/D49723.


================
Comment at: lib/Sema/SemaDecl.cpp:8189
+  const RecordType *RecTy =
+      PT->getPointeeOrArrayElementType()->getAs<RecordType>();
+  const RecordDecl *OrigRecDecl = RecTy->getDecl();
----------------
Anastasia wrote:
> yaxunl wrote:
> > Can we have a test for this change? e.g. an array of structs
> I am wondering if `PT->getPointeeOrArrayElementType()` is `nullptr`? Do we need to add an extra check?
Added in https://reviews.llvm.org/D49723.


================
Comment at: lib/Sema/SemaDecl.cpp:8189
+  const RecordType *RecTy =
+      PT->getPointeeOrArrayElementType()->getAs<RecordType>();
+  const RecordDecl *OrigRecDecl = RecTy->getDecl();
----------------
asavonic wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > Can we have a test for this change? e.g. an array of structs
> > I am wondering if `PT->getPointeeOrArrayElementType()` is `nullptr`? Do we need to add an extra check?
> Added in https://reviews.llvm.org/D49723.
It should not return null, unless the PT is null:
https://clang.llvm.org/doxygen/Type_8h_source.html#l06487



Repository:
  rC Clang

https://reviews.llvm.org/D49725