[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Anastasia Stulova via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6064f426a183: [OpenCL] Restrict addr space conversions in 
nested pointers (authored by Anastasia).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D73360?vs=242312=243132#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCLCXX/address-space-castoperators.cl
  clang/test/SemaOpenCLCXX/address-space-deduction.cl
  clang/test/SemaOpenCLCXX/address-space-references.cl

Index: clang/test/SemaOpenCLCXX/address-space-references.cl
===
--- clang/test/SemaOpenCLCXX/address-space-references.cl
+++ clang/test/SemaOpenCLCXX/address-space-references.cl
@@ -13,3 +13,16 @@
 void foo() {
   bar(1) // expected-error{{binding reference of type 'const __global unsigned int' to value of type 'int' changes address space}}
 }
+
+// Test addr space conversion with nested pointers
+
+extern void nestptr(int *&); // expected-note {{candidate function not viable: no known conversion from '__global int *__private' to '__generic int *__generic &__private' for 1st argument}}
+extern void nestptr_const(int * const &); // expected-note {{candidate function not viable: cannot pass pointer to address space '__constant' as a pointer to address space '__generic' in 1st argument}}
+int test_nestptr(__global int *glob, __constant int *cons, int* gen) {
+  nestptr(glob); // expected-error{{no matching function for call to 'nestptr'}}
+  // Addr space conversion first occurs on a temporary.
+  nestptr_const(glob);
+  // No legal conversion between disjoint addr spaces.
+  nestptr_const(cons); // expected-error{{no matching function for call to 'nestptr_const'}}
+  return *(*cons ? glob : gen);
+}
Index: clang/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- clang/test/SemaOpenCLCXX/address-space-deduction.cl
+++ clang/test/SemaOpenCLCXX/address-space-deduction.cl
@@ -105,7 +105,7 @@
 
 __kernel void k() {
   __local float x[2];
-  __local float(*p)[2];
+  float(*p)[2];
   t1(x);
   t2();
   t3();
Index: clang/test/SemaOpenCLCXX/address-space-castoperators.cl
===
--- /dev/null
+++ clang/test/SemaOpenCLCXX/address-space-castoperators.cl
@@ -0,0 +1,12 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+void nester_ptr() {
+  local int * * locgen;
+  constant int * * congen;
+  int * * gengen;
+
+  gengen = const_cast(locgen); //expected-error{{const_cast from '__local int *__generic *' to '__generic int *__generic *' is not allowed}}
+  gengen = static_cast(locgen); //expected-error{{static_cast from '__local int *__generic *' to '__generic int *__generic *' is not allowed}}
+// CHECK-NOT: AddressSpaceConversion
+  gengen = reinterpret_cast(locgen); //expected-warning{{reinterpret_cast from '__local int *__generic *' to '__generic int *__generic *' changes address space of nested pointers}}
+}
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -198,9 +198,7 @@
   ll = gg;   // expected-error {{assigning to '__local int *__private *' from incompatible type '__global int *__private *__private'}}
   ll = l;// expected-error {{assigning to '__local int *__private *' from incompatible type '__local int *__private'; take the address with &}}
   ll = gg_f; // expected-error {{assigning to '__local int *__private *' from incompatible type '__global float *__private *__private'}}
-  // FIXME: The below becomes a reinterpret_cast, and therefore does not emit an error
-  // even though the address space mismatches in the nested pointers.
-  ll = (__local int * __private *)gg;
+  ll = (__local int *__private *)gg; //expected-warning{{C-style cast from '__global int *__private *' to '__local int *__private *' changes address space of nested pointers}}
 
   gg_f = g;  // expected-error {{assigning to '__global float *__private *' from incompatible type '__global int *__private'}}
   gg_f = gg; // expected-error {{assigning to '__global float *__private *' from incompatible type '__global int *__private *__private'}}
Index: clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
===
--- clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
+++ clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
@@ -513,17 +513,37 @@
 #endif
 
   // Case 3: 

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D73360#1857073 , @rjmccall wrote:

> Okay, we can go with this for now, since it does fix a bug.


Thanks! Btw I uploaded straw-man patch for improving the diagnostics: 
https://reviews.llvm.org/D74116.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Okay, we can go with this for now, since it does fix a bug.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 242312.
Anastasia added a comment.

Add warning into `IncompatiblePointerTypesDiscardsQualifiers` group.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCLCXX/address-space-castoperators.cl
  clang/test/SemaOpenCLCXX/address-space-deduction.cl
  clang/test/SemaOpenCLCXX/address-space-references.cl

Index: clang/test/SemaOpenCLCXX/address-space-references.cl
===
--- clang/test/SemaOpenCLCXX/address-space-references.cl
+++ clang/test/SemaOpenCLCXX/address-space-references.cl
@@ -13,3 +13,16 @@
 void foo() {
   bar(1) // expected-error{{binding reference of type 'const __global unsigned int' to value of type 'int' changes address space}}
 }
+
+// Test addr space conversion with nested pointers
+
+extern void nestptr(int *&); // expected-note {{candidate function not viable: no known conversion from '__global int *__private' to '__generic int *__generic &__private' for 1st argument}}
+extern void nestptr_const(int * const &); // expected-note {{candidate function not viable: cannot pass pointer to address space '__constant' as a pointer to address space '__generic' in 1st argument}}
+int test_nestptr(__global int *glob, __constant int *cons, int* gen) {
+  nestptr(glob); // expected-error{{no matching function for call to 'nestptr'}}
+  // Addr space conversion first occurs on a temporary.
+  nestptr_const(glob);
+  // No legal conversion between disjoint addr spaces.
+  nestptr_const(cons); // expected-error{{no matching function for call to 'nestptr_const'}}
+  return *(*cons ? glob : gen);
+}
Index: clang/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- clang/test/SemaOpenCLCXX/address-space-deduction.cl
+++ clang/test/SemaOpenCLCXX/address-space-deduction.cl
@@ -105,7 +105,7 @@
 
 __kernel void k() {
   __local float x[2];
-  __local float(*p)[2];
+  float(*p)[2];
   t1(x);
   t2();
   t3();
Index: clang/test/SemaOpenCLCXX/address-space-castoperators.cl
===
--- /dev/null
+++ clang/test/SemaOpenCLCXX/address-space-castoperators.cl
@@ -0,0 +1,12 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+void nester_ptr() {
+  local int * * locgen;
+  constant int * * congen;
+  int * * gengen;
+
+  gengen = const_cast(locgen); //expected-error{{const_cast from '__local int *__generic *' to '__generic int *__generic *' is not allowed}}
+  gengen = static_cast(locgen); //expected-error{{static_cast from '__local int *__generic *' to '__generic int *__generic *' is not allowed}}
+// CHECK-NOT: AddressSpaceConversion
+  gengen = reinterpret_cast(locgen); //expected-warning{{reinterpret_cast from '__local int *__generic *' to '__generic int *__generic *' changes address space of nested pointers}}
+}
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -198,9 +198,7 @@
   ll = gg;   // expected-error {{assigning to '__local int *__private *' from incompatible type '__global int *__private *__private'}}
   ll = l;// expected-error {{assigning to '__local int *__private *' from incompatible type '__local int *__private'; take the address with &}}
   ll = gg_f; // expected-error {{assigning to '__local int *__private *' from incompatible type '__global float *__private *__private'}}
-  // FIXME: The below becomes a reinterpret_cast, and therefore does not emit an error
-  // even though the address space mismatches in the nested pointers.
-  ll = (__local int * __private *)gg;
+  ll = (__local int *__private *)gg; //expected-warning{{C-style cast from '__global int *__private *' to '__local int *__private *' changes address space of nested pointers}}
 
   gg_f = g;  // expected-error {{assigning to '__global float *__private *' from incompatible type '__global int *__private'}}
   gg_f = gg; // expected-error {{assigning to '__global float *__private *' from incompatible type '__global int *__private *__private'}}
Index: clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
===
--- clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
+++ clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
@@ -513,17 +513,37 @@
 #endif
 
   // Case 3: Corresponded inner pointees has overlapping but not equivalent address spaces.
-  // FIXME: Should this really be allowed in C++ mode?
   var_as_as_int = var_asc_asc_int;
-#if !__OPENCL_CPP_VERSION__
 

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D73360#1852147 , @rjmccall wrote:

> If it's reasonable to persist the failure kind, that would probably be good, 
> but it might be a lot of work.


I have looked at it earlier and it indeed affects a lot of Sema in C++ mode. 
Besides amount of work I am not sure I understand the flow enough to come up 
with a good approach.

Maybe we can finalize this review first to fix the regression. Because I would 
like it to be ported to the release 10 branch too. Then I can spend some time 
to see if I can come up with something better for the diagnostic too. :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

If it's reasonable to persist the failure kind, that would probably be good, 
but it might be a lot of work.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D73360#1850001 , @rjmccall wrote:

> Is there no follow-up code when actually emitting the failure diagnostic 
> which tries to figure out a more specific cause of failure?


After looking at this a bit more it seems C++ part calls the diagnostic code 
earlier:

  Sema::PerformImplicitConversion:
  
 case ImplicitConversionSequence::BadConversion:
   bool Diagnosed =
   DiagnoseAssignmentResult(Incompatible, From->getExprLoc(), ToType,
From->getType(), From, Action);

It just seems to always pass `Incompatible`. We could add follow up code 
before? This would be easier than modifying the existing flow completely. 
However, looking at this holistically we would need to repeat all checks 
classifying the failure types...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Is there no follow-up code when actually emitting the failure diagnostic which 
tries to figure out a more specific cause of failure?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia marked an inline comment as done.
Anastasia added inline comments.



Comment at: clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl:521
+#else
+// expected-error@-5 {{assigning to '__generic int *__generic *' from 
incompatible type '__local int *__local *__private'}}
 #endif

rjmccall wrote:
> Can we specialize the diagnostic here so that we get the good diagnostic in 
> both language modes?
I am not sure of the approach so I am wondering if you have any good 
suggestions.

It seems in C++ Clang exits with quite generic `Incompatible` in the implicit 
conversion case because the functionality for doing the semantic checks doesn't 
generally set `Sema::AssignConvertType`.

https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html

```

Sema::CheckSingleAssignmentConstraints:


  if (getLangOpts().CPlusPlus) {
  if (!LHSType->isRecordType() && !LHSType->isAtomicType()) {
// C++ 5.17p3: If the left operand is not of class type, the
// expression is implicitly converted (C++ 4) to the
// cv-unqualified type of the left operand.
QualType RHSType = RHS.get()->getType();
if (Diagnose) {
  RHS = PerformImplicitConversion(RHS.get(), 
LHSType.getUnqualifiedType(),
  AA_Assigning);
} else {
  ...
}
if (RHS.isInvalid())
  return Incompatible;
```

However for example in `IsStandardConversion`  I can see that 
`Sema::AssignConvertType` is being set but only for classifying the conversion 
sequence. It is never propagated outside.

http://clang.llvm.org/doxygen/SemaOverload_8cpp_source.html


```
IsStandardConversion:


ExprResult ER = ExprResult{From};
Sema::AssignConvertType Conv =
S.CheckSingleAssignmentConstraints(ToType, ER,
   /*Diagnose=*/false,
   /*DiagnoseCFAudited=*/false,
   /*ConvertRHS=*/false);
ImplicitConversionKind SecondConv;
switch (Conv) {
case Sema::Compatible:
  SecondConv = ICK_C_Only_Conversion;
  break;
// For our purposes, discarding qualifiers is just as bad as using an
// incompatible pointer. Note that an IncompatiblePointer conversion can 
drop
// qualifiers, as well.
case Sema::CompatiblePointerDiscardsQualifiers:
case Sema::IncompatiblePointer:
case Sema::IncompatiblePointerSign:
  SecondConv = ICK_Incompatible_Pointer_Conversion;
  break;
default:
  return false;
}


```

In most of other places it's not being used for C++ mode. I feel like C++ flow 
was not written to set the conversion type failure. Maybe there is something 
else I could use in C++ mode to specialize this diagnostic? Otherwise, it seems 
like quite a lot of Sema would have to be modified for C++ flow because 
implicit conversions are used in many different places.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Jeroen Dobbelaere via Phabricator via cfe-commits]

jeroen.dobbelaere accepted this revision.
jeroen.dobbelaere added a comment.

This patch looks good to me.

I do agree with John that it would be good to add the warning to a warning 
group.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[John McCall via Phabricator via cfe-commits]

rjmccall requested changes to this revision.
rjmccall added inline comments.
This revision now requires changes to proceed.



Comment at: clang/test/Misc/warning-flags.c:41
 CHECK-NEXT:   warn_asm_label_on_auto_decl
+CHECK-NEXT:   warn_bad_cxx_cast_nested_pointer_addr_space
 CHECK-NEXT:   warn_c_kext

Please give this warning  a warning group rather than changing this test.



Comment at: clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl:521
+#else
+// expected-error@-5 {{assigning to '__generic int *__generic *' from 
incompatible type '__local int *__local *__private'}}
 #endif

Can we specialize the diagnostic here so that we get the good diagnostic in 
both language modes?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73360/new/

https://reviews.llvm.org/D73360



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: rjmccall, jeroen.dobbelaere, bevinh.
Herald added subscribers: ebevhan, yaxunl.

This patch is fixing the issue reported in:
http://lists.llvm.org/pipermail/cfe-dev/2020-January/064273.html
and in the bug:
https://bugs.llvm.org/show_bug.cgi?id=39674

Since address space conversion changes pointer representation it is not legal 
to do it for the nested pointers even with compatible address spaces. Because 
the address space conversion in the nested levels can't be generated. The 
behavior implemented by this patch is as follows:

- Reject any implicit conversions of nested pointers with address spaces.
- Reject address space of nested pointers conversions in safe casts e.g. 
`const_cast` or `static_cast.`
- Allow conversion in low level C-style or `reinterpret_cast` but with a 
warning (this aligns with OpenCL C behavior).


https://reviews.llvm.org/D73360

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/Misc/warning-flags.c
  clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
  clang/test/SemaOpenCL/address-spaces.cl
  clang/test/SemaOpenCLCXX/address-space-castoperators.cl
  clang/test/SemaOpenCLCXX/address-space-deduction.cl
  clang/test/SemaOpenCLCXX/address-space-references.cl

Index: clang/test/SemaOpenCLCXX/address-space-references.cl
===
--- clang/test/SemaOpenCLCXX/address-space-references.cl
+++ clang/test/SemaOpenCLCXX/address-space-references.cl
@@ -13,3 +13,16 @@
 void foo() {
   bar(1) // expected-error{{binding reference of type 'const __global unsigned int' to value of type 'int' changes address space}}
 }
+
+// Test addr space conversion with nested pointers
+
+extern void nestptr(int *&); // expected-note {{candidate function not viable: no known conversion from '__global int *__private' to '__generic int *__generic &__private' for 1st argument}}
+extern void nestptr_const(int * const &); // expected-note {{candidate function not viable: cannot pass pointer to address space '__constant' as a pointer to address space '__generic' in 1st argument}}
+int test_nestptr(__global int *glob, __constant int *cons, int* gen) {
+  nestptr(glob); // expected-error{{no matching function for call to 'nestptr'}}
+  // Addr space conversion first occurs on a temporary.
+  nestptr_const(glob);
+  // No legal conversion between disjoint addr spaces.
+  nestptr_const(cons); // expected-error{{no matching function for call to 'nestptr_const'}}
+  return *(*cons ? glob : gen);
+}
Index: clang/test/SemaOpenCLCXX/address-space-deduction.cl
===
--- clang/test/SemaOpenCLCXX/address-space-deduction.cl
+++ clang/test/SemaOpenCLCXX/address-space-deduction.cl
@@ -105,7 +105,7 @@
 
 __kernel void k() {
   __local float x[2];
-  __local float(*p)[2];
+  float(*p)[2];
   t1(x);
   t2();
   t3();
Index: clang/test/SemaOpenCLCXX/address-space-castoperators.cl
===
--- /dev/null
+++ clang/test/SemaOpenCLCXX/address-space-castoperators.cl
@@ -0,0 +1,12 @@
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+
+void nester_ptr() {
+  local int * * locgen;
+  constant int * * congen;
+  int * * gengen;
+
+  gengen = const_cast(locgen); //expected-error{{const_cast from '__local int *__generic *' to '__generic int *__generic *' is not allowed}}
+  gengen = static_cast(locgen); //expected-error{{static_cast from '__local int *__generic *' to '__generic int *__generic *' is not allowed}}
+// CHECK-NOT: AddressSpaceConversion
+  gengen = reinterpret_cast(locgen); //expected-warning{{reinterpret_cast from '__local int *__generic *' to '__generic int *__generic *' changes address space of nested pointers}}
+}
Index: clang/test/SemaOpenCL/address-spaces.cl
===
--- clang/test/SemaOpenCL/address-spaces.cl
+++ clang/test/SemaOpenCL/address-spaces.cl
@@ -198,9 +198,7 @@
   ll = gg;   // expected-error {{assigning to '__local int *__private *' from incompatible type '__global int *__private *__private'}}
   ll = l;// expected-error {{assigning to '__local int *__private *' from incompatible type '__local int *__private'; take the address with &}}
   ll = gg_f; // expected-error {{assigning to '__local int *__private *' from incompatible type '__global float *__private *__private'}}
-  // FIXME: The below becomes a reinterpret_cast, and therefore does not emit an error
-  // even though the address space mismatches in the nested pointers.
-  ll = (__local int * __private *)gg;
+  ll = (__local int *__private *)gg; //expected-warning{{C-style cast from '__global int *__private *' to '__local int *__private *' changes address space of nested pointers}}
 
   gg_f = g;  // expected-error {{assigning to '__global