Page MenuHomePhabricator

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

Authored by Anastasia on May 20 2019, 11:19 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

Anastasia created this revision.May 20 2019, 11:19 AM
Anastasia edited the summary of this revision. (Show Details)May 20 2019, 11:20 AM
Anastasia marked 3 inline comments as done.
Anastasia added inline comments.
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

kpet added a subscriber: kpet.May 20 2019, 11:47 AM
rjmccall added inline comments.May 20 2019, 1:00 PM
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.

  • Switched back to loop over all method qualifiers
  • Moved addr space check into AddOverloadCandidate
Anastasia marked 2 inline comments as done.May 23 2019, 10:38 AM
Anastasia retitled this revision from [Sema] Diagnose addr space mismatch while constructing objects to [Sema][PR41730] Diagnose addr space mismatch while constructing objects.May 23 2019, 11:21 AM
rjmccall added inline comments.May 23 2019, 11:38 AM
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 updated this revision to Diff 201228.May 24 2019, 6:55 AM
Anastasia marked an inline comment as done.
  • Added candidates as not viable
  • Added dedicated diagnostic note
  • Added assert
Anastasia added inline comments.May 24 2019, 6:56 AM
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 added inline comments.May 24 2019, 10:08 AM
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 updated this revision to Diff 202193.May 30 2019, 7:43 AM
Anastasia marked 3 inline comments as done.
  • Improved diagnostic
  • Added more comments
Anastasia marked 4 inline comments as done.May 30 2019, 7:46 AM
Anastasia added inline comments.
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 added inline comments.May 30 2019, 7:24 PM
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 updated this revision to Diff 202690.Jun 3 2019, 4:32 AM
Anastasia marked an inline comment as done.

Changed FIXME to a regular comment, modified wording.

Anastasia marked 2 inline comments as done.Jun 3 2019, 4:34 AM
Anastasia added inline comments.
lib/Sema/SemaOverload.cpp
6095 ↗(On Diff #201228)

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

rjmccall accepted this revision.Jun 10 2019, 3:22 PM

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.

This revision is now accepted and ready to land.Jun 10 2019, 3:22 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 9:20 AM
Anastasia marked 2 inline comments as done.Jun 20 2019, 9:42 AM
Anastasia added inline comments.
test/SemaCXX/address-space-ctor.cpp
11 ↗(On Diff #202193)

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