[Sema][PR41730] Diagnose addr space mismatch while constructing objects

[Anastasia]

Summary

If we construct an object in some arbitrary non-default addr space it should fail unless either:

1. There is an implicit conversion from the address space to default/generic address space.
2. There is a matching ctor qualified with an address space that is either exactly matching or convertible to the address space of an object.

Example - case 1:

struct MyType {
   MyType(int i)  : i(i) {}
   int i;
 };
__constant MyType m(1); // error: can't convert from __constant to __generic

Example - case 2:

struct MyType {
   MyType(int i) __constant  : i(i) {}
   MyType(int i)  : i(i) {}
   int i;
 };
__constant MyType c(1); // there is __constant qualified ctor
__global MyType g(1); // there is a valid conversion between __global and __generic

[Anastasia]

lib/Sema/SemaInit.cpp

3771

matches

5023

matches

test/CodeGenOpenCLCXX/addrspace-ctor.cl

7

remove unused bar

[rjmccall]

lib/Sema/SemaDeclCXX.cpp

8229

We want to catch _Atomic, too, so please just change this loop to ignore address-space qualifiers, using a flag to decide whether to call setInvalidType.

lib/Sema/SemaInit.cpp

3773

Please add these constructors as candidates and then teach AddOverloadCandidate to reject them. If you need to set the destination address space on the OverloadCandidateSet, I think that would be reasonable.

[Anastasia]

• Switched back to loop over all method qualifiers
• Moved addr space check into AddOverloadCandidate

[rjmccall]

include/clang/Sema/Overload.h

977 ↗

(On Diff #201011)

Can this assert that Kind == CSK_InitByConstructor || Kind == CSK_InitByUserDefinedConversion?

lib/Sema/SemaDeclCXX.cpp

8229

If there aren't any qualifiers we're skipping, the flag isn't necessary.

lib/Sema/SemaOverload.cpp

6038 ↗

(On Diff #201011)

I think you should add the candidate but mark it as non-viable.

[Anastasia]

• Added candidates as not viable
• Added dedicated diagnostic note
• Added assert

[Anastasia]

lib/Sema/SemaDeclCXX.cpp

8229

We are skipping addr space currently. I use this flag to avoid setting declaration as invalid below if it's only qualified by an addr space.

[rjmccall]

include/clang/Basic/DiagnosticSemaKinds.td

3653 ↗

(On Diff #201228)

"candidate constructor ignored: cannot be used to construct an object in address space %0"?

lib/Sema/SemaDeclCXX.cpp

8229

Okay. Does forEachQualifier not visit address-space qualifiers?

Please leave a comment explaining that that's the intended behavior: we should visit everything except an address space.

lib/Sema/SemaOverload.cpp

6095 ↗

(On Diff #201228)

"Check that the constructor is capable of constructing an object in the destination address space."

Should there be an exception here for trivial default/copy/move constructors?

test/CodeGenCXX/address-space-of-this.cpp

3

Is there nothing in this test that's worth continuing to check while we work on fixing this problem?

[Anastasia]

• Improved diagnostic
• Added more comments

[Anastasia]

lib/Sema/SemaDeclCXX.cpp

8229

Okay. Does forEachQualifier not visit address-space qualifiers?

It calls forEachCVRUQualifier, nothing else.

Please leave a comment explaining that that's the intended behavior: we should visit everything except an address space.

Added in FIXME.

lib/Sema/SemaOverload.cpp

6095 ↗

(On Diff #201228)

Good point. Current implementation is generating one overload of each default/copy/move in generic address space only. But we could potentially look at changing this. If we could add extra overloads once we encounter each new ctor invocation it would be the easiest approach and this code would still be applicable. However, I would need to understand the feasibility in more details. May be this is something for the future work? Or do you have other suggestions?

test/CodeGenCXX/address-space-of-this.cpp

3

We can only compile this file if we had address space qualifiers accepted on methods, but it's still WIP (https://reviews.llvm.org/D57464) and I don't have the time to fix it now. But at the same time the OpenCL test cover the functionality originally intended in this test. Not sure if it's better to remove this test completely?

test/SemaCXX/address-space-ctor.cpp

12

Not sure if we should change to:

cannot be used to construct an object with '__attribute__((address_space(10)))'

Although for OpenCL it would be ok as is.

Or may be it's better to add custom printing of addr spaces?

[rjmccall]

lib/Sema/SemaDeclCXX.cpp

8233

I don't think this is a FIXME; it's just a notice to future maintainers. How about something like: "We want to emit a diagnostic on any qualifier except an address-space qualifier. However, forEachQualifier currently doesn't visit address-space qualifiers, so there's no way to write this condition right now; we just diagnose on everything."

lib/Sema/SemaOverload.cpp

6095 ↗

(On Diff #201228)

It's fine to leave it as future work. I think synthesizing new overloads after the fact will probably be problematic for the AST, though, and it'll be easier to just treat trivial copy/move constructors (and assignment operators, for that matter) as implicitly being able to construct objects in all possible address spaces (as well as *from* all possible address spaces).

[Anastasia]

Changed FIXME to a regular comment, modified wording.

[Anastasia]

lib/Sema/SemaOverload.cpp

6095 ↗

(On Diff #201228)

Ok, thanks! I will look into this later as a separate patch then.

[rjmccall]

Okay, LGTM.

test/SemaCXX/address-space-ctor.cpp

12

Some sort of custom printing of address spaces would probably be best, yeah. Users probably think of them as whatever custom syntax/macro they normally write, not as an application of some weird attribute. We do a similar sort of thing with vector types where we generally just print them as float4 or whatever without an aka clause because we don't expect anyone to know about those attributes.

[Anastasia]

test/SemaCXX/address-space-ctor.cpp

12

Ok, sure. I will look into it later. Thanks!

include/clang/Sema/DeclSpec.h

struct FunctionTypeInfo {
     bool hasMutableQualifier() const { return getMutableLoc().isValid(); }
 
     /// Determine whether this method has qualifiers.
     bool hasMethodTypeQualifiers() const {
       return MethodQualifiers && (MethodQualifiers->getTypeQualifiers() ||
                                   MethodQualifiers->getAttributes().size());
     }
 
+    /// Determine whether this method has CVRU qualifiers.
+    bool hasMethodTypeCVRUQualifiers() {
+      if (!MethodQualifiers)
+        return false;
+      auto Quals = MethodQualifiers->getTypeQualifiers();
+      return Quals & DeclSpec::TQ_const || Quals & DeclSpec::TQ_volatile ||
+             Quals & DeclSpec::TQ_restrict || Quals & DeclSpec::TQ_unaligned;
+    }
+
     /// Get the type of exception specification this function has.
     ExceptionSpecificationType getExceptionSpecType() const {
       return static_cast<ExceptionSpecificationType>(ExceptionSpecType);
     }
 
     /// Get the number of dynamic exception specifications.
     unsigned getNumExceptions() const {
       assert(ExceptionSpecType != EST_None);

lib/Sema/SemaDeclCXX.cpp

diagnoseIgnoredQualifiers(
         diag::err_constructor_return_type, TypeQuals, SourceLocation(),
         D.getDeclSpec().getConstSpecLoc(), D.getDeclSpec().getVolatileSpecLoc(),
         D.getDeclSpec().getRestrictSpecLoc(),
         D.getDeclSpec().getAtomicSpecLoc());
     D.setInvalidType();
   }
 
   DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
-  if (FTI.hasMethodTypeQualifiers()) {
-    FTI.MethodQualifiers->forEachQualifier(
+  if (FTI.hasMethodTypeCVRUQualifiers()) {
+    FTI.MethodQualifiers->forEachCVRUQualifier(
         [&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) {
           Diag(SL, diag::err_invalid_qualified_constructor)
               << QualName << SourceRange(SL);
         });
     D.setInvalidType();
   }
 
   // C++0x [class.ctor]p4:
   //   A constructor shall not be declared with a ref-qualifier.
   if (FTI.hasRefQualifier()) {
     Diag(FTI.getRefQualifierLoc(), diag::err_ref_qualifier_constructor)
       << FTI.RefQualifierIsLValueRef

lib/Sema/SemaInit.cpp

bool SuppressUserConversions =
         (IsListInit && Args.size() == 1 && isa<InitListExpr>(Args[0]) &&
          hasCopyOrMoveCtorParam(S.Context, Info));
 
     if (Info.ConstructorTmpl)
       S.AddTemplateOverloadCandidate(Info.ConstructorTmpl, Info.FoundDecl,
                                      /*ExplicitArgs*/ nullptr, Args,
                                      CandidateSet, SuppressUserConversions);
     else {
+      // Check that address space match to resolve the constructors correctly.
+      if (Info.Constructor->getMethodQualifiers().isAddressSpaceSupersetOf(
+              DestType.getQualifiers())) {
         // C++ [over.match.copy]p1:
         //   - When initializing a temporary to be bound to the first parameter
         //     of a constructor [for type T] that takes a reference to possibly
         //     cv-qualified T as its first argument, called with a single
         //     argument in the context of direct-initialization, explicit
         //     conversion functions are also considered.
-      // FIXME: What if a constructor template instantiates to such a signature?
+        // FIXME: What if a constructor template instantiates to such a
+        // signature?
         bool AllowExplicitConv = AllowExplicit && !CopyInitializing &&
                                  Args.size() == 1 &&
                                  hasCopyOrMoveCtorParam(S.Context, Info);
         S.AddOverloadCandidate(Info.Constructor, Info.FoundDecl, Args,
                                CandidateSet, SuppressUserConversions,
                                /*PartialOverloading=*/false,
                                /*AllowExplicit=*/AllowExplicitConv);
       }
     }
+  }
 
   // FIXME: Work around a bug in C++17 guaranteed copy elision.
   //
   // When initializing an object of class type T by constructor
   // ([over.match.ctor]) or by list-initialization ([over.match.list])
   // from a single expression of class type U, conversion functions of
   // U that convert to the non-reference type cv T are candidates.
   // Explicit conversion functions are only candidates during
if (S.isCompleteType(Kind.getLocation(), DestType)) {
         if (!Info.Constructor->isInvalidDecl() &&
             Info.Constructor->isConvertingConstructor(AllowExplicit)) {
           if (Info.ConstructorTmpl)
             S.AddTemplateOverloadCandidate(Info.ConstructorTmpl, Info.FoundDecl,
                                            /*ExplicitArgs*/ nullptr,
                                            Initializer, CandidateSet,
                                            /*SuppressUserConversions=*/true);
           else
+              // Check that address space match to resolve the constructors
+              // correctly.
+              if (Info.Constructor->getMethodQualifiers()
+                      .isAddressSpaceSupersetOf(DestType.getQualifiers()))
             S.AddOverloadCandidate(Info.Constructor, Info.FoundDecl,
                                    Initializer, CandidateSet,
                                    /*SuppressUserConversions=*/true);
         }
       }
     }
   }
 

test/CodeGenCXX/address-space-of-this.cpp

 // RUN: %clang_cc1 %s -std=c++14 -triple=spir -emit-llvm -o - | FileCheck %s
 // RUN: %clang_cc1 %s -std=c++17 -triple=spir -emit-llvm -o - | FileCheck %s
+// XFAIL: *
 
+// FIXME: We can't compile address space method qualifiers yet.
+// Therefore there is no way to check correctness of this code.
 struct MyType {
-  MyType(int i) : i(i) {}
+  MyType(int i) __attribute__((address_space(10))) : i(i) {}
   int i;
 };
 //CHECK: call void @_ZN6MyTypeC1Ei(%struct.MyType* addrspacecast (%struct.MyType addrspace(10)* @m to %struct.MyType*), i32 123)
 MyType __attribute__((address_space(10))) m = 123;

test/CodeGenOpenCLCXX/addrspace-ctor.cl

+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct MyType {
+  MyType(int i) : i(i) {}
+  MyType(int i) __constant : i(i) {}
+  int i;
+  void bar();
+};
+
+//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const1, i32 1)
+__constant MyType const1 = 1;
+//CHECK: call void @_ZNU3AS26MyTypeC1Ei(%struct.MyType addrspace(2)* @const2, i32 2)
+__constant MyType const2(2);
+//CHECK: call void @_ZNU3AS46MyTypeC1Ei(%struct.MyType addrspace(4)* addrspacecast (%struct.MyType addrspace(1)* @glob to %struct.MyType addrspace(4)*), i32 1)
+MyType glob(1);

test/SemaCXX/address-space-ctor.cpp

+// RUN: %clang_cc1 %s -std=c++14 -triple=spir -verify -fsyntax-only
+// RUN: %clang_cc1 %s -std=c++17 -triple=spir -verify -fsyntax-only
+
+struct MyType {
+  MyType(int i) : i(i) {}
+  int i;
+};
+
+// FIXME: We can't implicitly convert between address spaces yet.
+MyType __attribute__((address_space(10))) m1 = 123; //expected-error{{no viable conversion from 'int' to '__attribute__((address_space(10))) MyType'}}
+MyType __attribute__((address_space(10))) m2(123);  //expected-error{{no matching constructor for initialization of '__attribute__((address_space(10))) MyType'}}