[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 ↗

(On Diff #200338)

matches

5023 ↗

(On Diff #200338)

matches

test/CodeGenOpenCLCXX/addrspace-ctor.cl

7 ↗

(On Diff #200338)

remove unused bar

[rjmccall]

lib/Sema/SemaDeclCXX.cpp

8229 ↗

(On Diff #200338)

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 ↗

(On Diff #200338)

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 ↗

(On Diff #200338)

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 ↗

(On Diff #200338)

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 ↗

(On Diff #200338)

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 ↗

(On Diff #201228)

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 ↗

(On Diff #200338)

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 ↗

(On Diff #201228)

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

11 ↗

(On Diff #202193)

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

8219 ↗

(On Diff #202193)

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

11 ↗

(On Diff #202193)

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

11 ↗

(On Diff #202193)

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

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def note_ovl_candidate_inherited_constructor : Note<
     "constructor from base class %0 inherited here">;
 def note_ovl_candidate_inherited_constructor_slice : Note<
     "candidate %select{constructor|template}0 ignored: "
     "inherited constructor cannot be used to %select{copy|move}1 object">;
 def note_ovl_candidate_illegal_constructor : Note<
     "candidate %select{constructor|template}0 ignored: "
     "instantiation %select{takes|would take}0 its own class type by value">;
+def note_ovl_candidate_illegal_constructor_adrspace_mismatch : Note<
+    "candidate constructor ignored: cannot be used to construct an object "
+    "in address space %0">;
 def note_ovl_candidate_bad_deduction : Note<
     "candidate template ignored: failed template argument deduction">;
 def note_ovl_candidate_incomplete_deduction : Note<"candidate template ignored: "
     "couldn't infer template argument %0">;
 def note_ovl_candidate_incomplete_deduction_pack : Note<"candidate template ignored: "
     "deduced too few arguments for expanded pack %0; no argument for %ordinal1 "
     "expanded parameter in deduced argument pack %2">;
 def note_ovl_candidate_inconsistent_deduction : Note<

cfe/trunk/include/clang/Sema/Overload.h

enum OverloadFailureKind {
 
     /// This inherited constructor is not viable because it would slice the
     /// argument.
     ovl_fail_inhctor_slice,
 
     /// This candidate was not viable because it is a non-default multiversioned
     /// function.
     ovl_non_default_multiversion_function,
+
+    /// This constructor/conversion candidate fail due to an address space
+    /// mismatch between the object being constructed and the overload
+    /// candidate.
+    ovl_fail_object_addrspace_mismatch
   };
 
   /// A list of implicit conversion sequences for the arguments of an
   /// OverloadCandidate.
   using ConversionSequenceList =
       llvm::MutableArrayRef<ImplicitConversionSequence>;
 
   /// OverloadCandidate - A single candidate in an overload set (C++ 13.3).
private:
     SourceLocation Loc;
     CandidateSetKind Kind;
 
     constexpr static unsigned NumInlineBytes =
         24 * sizeof(ImplicitConversionSequence);
     unsigned NumInlineBytesUsed = 0;
     llvm::AlignedCharArray<alignof(void *), NumInlineBytes> InlineSpace;
 
+    // Address space of the object being constructed.
+    LangAS DestAS = LangAS::Default;
+
     /// If we have space, allocates from inline storage. Otherwise, allocates
     /// from the slab allocator.
     /// FIXME: It would probably be nice to have a SmallBumpPtrAllocator
     /// instead.
     /// FIXME: Now that this only allocates ImplicitConversionSequences, do we
     /// want to un-generalize this?
     template <typename T>
     T *slabAllocate(unsigned N) {
void NoteCandidates(
         SourceLocation Loc = SourceLocation(),
         llvm::function_ref<bool(OverloadCandidate &)> Filter =
             [](OverloadCandidate &) { return true; });
 
     void NoteCandidates(Sema &S, ArrayRef<Expr *> Args,
                         ArrayRef<OverloadCandidate *> Cands,
                         StringRef Opc = "",
                         SourceLocation OpLoc = SourceLocation());
+
+    LangAS getDestAS() { return DestAS; }
+
+    void setDestAS(LangAS AS) {
+      assert((Kind == CSK_InitByConstructor ||
+              Kind == CSK_InitByUserDefinedConversion) &&
+             "can't set the destination address space when not constructing an "
+             "object");
+      DestAS = AS;
+    }
+
   };
 
   bool isBetterOverloadCandidate(Sema &S,
                                  const OverloadCandidate &Cand1,
                                  const OverloadCandidate &Cand2,
                                  SourceLocation Loc,
                                  OverloadCandidateSet::CandidateSetKind Kind);
 

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

diagnoseIgnoredQualifiers(
         D.getDeclSpec().getConstSpecLoc(), D.getDeclSpec().getVolatileSpecLoc(),
         D.getDeclSpec().getRestrictSpecLoc(),
         D.getDeclSpec().getAtomicSpecLoc());
     D.setInvalidType();
   }
 
   DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
   if (FTI.hasMethodTypeQualifiers()) {
+    bool DiagOccured = false;
     FTI.MethodQualifiers->forEachQualifier(
         [&](DeclSpec::TQ TypeQual, StringRef QualName, SourceLocation SL) {
+          // This diagnostic should be emitted on any qualifier except an addr
+          // space qualifier. However, forEachQualifier currently doesn't visit
+          // addr space qualifiers, so there's no way to write this condition
+          // right now; we just diagnose on everything.
           Diag(SL, diag::err_invalid_qualified_constructor)
               << QualName << SourceRange(SL);
+          DiagOccured = true;
         });
+    if (DiagOccured)
       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
       << FixItHint::CreateRemoval(FTI.getRefQualifierLoc());

cfe/trunk/lib/Sema/SemaInit.cpp

ResolveConstructorOverload(Sema &S, SourceLocation DeclLoc,
                            OverloadCandidateSet &CandidateSet,
                            QualType DestType,
                            DeclContext::lookup_result Ctors,
                            OverloadCandidateSet::iterator &Best,
                            bool CopyInitializing, bool AllowExplicit,
                            bool OnlyListConstructors, bool IsListInit,
                            bool SecondStepOfCopyInit = false) {
   CandidateSet.clear(OverloadCandidateSet::CSK_InitByConstructor);
+  CandidateSet.setDestAS(DestType.getQualifiers().getAddressSpace());
 
   for (NamedDecl *D : Ctors) {
     auto Info = getConstructorInfo(D);
     if (!Info.Constructor || Info.Constructor->isInvalidDecl())
       continue;
 
     if (!AllowExplicit && Info.Constructor->isExplicit())
       continue;
static void TryUserDefinedConversion(Sema &S,
   QualType SourceType = Initializer->getType();
   assert((DestType->isRecordType() || SourceType->isRecordType()) &&
          "Must have a class type to perform a user-defined conversion");
 
   // Build the candidate set directly in the initialization sequence
   // structure, so that it will persist if we fail.
   OverloadCandidateSet &CandidateSet = Sequence.getFailedCandidateSet();
   CandidateSet.clear(OverloadCandidateSet::CSK_InitByUserDefinedConversion);
+  CandidateSet.setDestAS(DestType.getQualifiers().getAddressSpace());
 
   // Determine whether we are allowed to call explicit constructors or
   // explicit conversion operators.
   bool AllowExplicit = Kind.AllowExplicit();
 
   if (const RecordType *DestRecordType = DestType->getAs<RecordType>()) {
     // The type we're converting to is a class type. Enumerate its constructors
     // to see if there is a suitable conversion.

cfe/trunk/lib/Sema/SemaOverload.cpp

if (Shadow && Args.size() == 1 && Constructor->getNumParams() >= 1 &&
       SourceLocation Loc = Args.front()->getExprLoc();
       if ((Context.hasSameUnqualifiedType(P, C) || IsDerivedFrom(Loc, P, C)) &&
           (Context.hasSameUnqualifiedType(D, P) || IsDerivedFrom(Loc, D, P))) {
         Candidate.Viable = false;
         Candidate.FailureKind = ovl_fail_inhctor_slice;
         return;
       }
     }
+
+    // Check that the constructor is capable of constructing an object in the
+    // destination address space.
+    if (!Qualifiers::isAddressSpaceSupersetOf(
+            Constructor->getMethodQualifiers().getAddressSpace(),
+            CandidateSet.getDestAS())) {
+      Candidate.Viable = false;
+      Candidate.FailureKind = ovl_fail_object_addrspace_mismatch;
+    }
   }
 
   unsigned NumParams = Proto->getNumParams();
 
   // (C++ 13.3.2p2): A candidate function having fewer than m
   // parameters is viable only if it has an ellipsis in its parameter
   // list (8.3.5).
   if (TooManyArguments(NumParams, Args.size(), PartialOverloading) &&

 /// major challenges of technical writing.  Yes, this makes pointing
 /// out problems with specific arguments quite awkward.  It's still
 /// better than generating twenty screens of text for every failed
 /// overload.
 ///
 /// It would be great to be able to express per-candidate problems
 /// more richly for those diagnostic clients that cared, but we'd
 /// still have to be just as careful with the default diagnostics.
+/// \param CtorDestAS Addr space of object being constructed (for ctor
+/// candidates only).
 static void NoteFunctionCandidate(Sema &S, OverloadCandidate *Cand,
                                   unsigned NumArgs,
-                                  bool TakingCandidateAddress) {
+                                  bool TakingCandidateAddress,
+                                  LangAS CtorDestAS = LangAS::Default) {
   FunctionDecl *Fn = Cand->Function;
 
   // Note deleted candidates, but only if they're viable.
   if (Cand->Viable) {
     if (Fn->isDeleted()) {
       std::string FnDesc;
       std::pair<OverloadCandidateKind, OverloadCandidateSelect> FnKindPair =
           ClassifyOverloadCandidate(S, Cand->FoundDecl, Fn, FnDesc);
static void NoteFunctionCandidate(Sema &S, OverloadCandidate *Cand,
 
   case ovl_fail_illegal_constructor: {
     S.Diag(Fn->getLocation(), diag::note_ovl_candidate_illegal_constructor)
       << (Fn->getPrimaryTemplate() ? 1 : 0);
     MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
     return;
   }
 
+  case ovl_fail_object_addrspace_mismatch: {
+    Qualifiers QualsForPrinting;
+    QualsForPrinting.setAddressSpace(CtorDestAS);
+    S.Diag(Fn->getLocation(),
+           diag::note_ovl_candidate_illegal_constructor_adrspace_mismatch)
+        << QualsForPrinting;
+    MaybeEmitInheritedConstructorNote(S, Cand->FoundDecl);
+    return;
+  }
+
   case ovl_fail_trivial_conversion:
   case ovl_fail_bad_final_conversion:
   case ovl_fail_final_conversion_not_exact:
     return S.NoteOverloadCandidate(Cand->FoundDecl, Fn);
 
   case ovl_fail_bad_conversion: {
     unsigned I = (Cand->IgnoreObjectArgument ? 1 : 0);
     for (unsigned N = Cand->Conversions.size(); I != N; ++I)
for (; I != E; ++I) {
     // candidate list.
     if (CandsShown >= 4 && ShowOverloads == Ovl_Best) {
       break;
     }
     ++CandsShown;
 
     if (Cand->Function)
       NoteFunctionCandidate(S, Cand, Args.size(),
-                            /*TakingCandidateAddress=*/false);
+                            /*TakingCandidateAddress=*/false, DestAS);
     else if (Cand->IsSurrogate)
       NoteSurrogateCandidate(S, Cand);
     else {
       assert(Cand->Viable &&
              "Non-viable built-in candidates are not added to Cands.");
       // Generally we only see ambiguities including viable builtin
       // operators if overload resolution got screwed up by an
       // ambiguous user-defined conversion.

cfe/trunk/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;

cfe/trunk/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;
+};
+
+//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);

cfe/trunk/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;
+};
+
+//expected-note@-5{{candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const MyType &' for 1st argument}}
+//expected-note@-6{{candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to 'MyType &&' for 1st argument}}
+//expected-note@-6{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
+//expected-note@-8{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
+//expected-note@-9{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
+//expected-note@-9{{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(10)))'}}
+
+// 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'}}