[clang] [clang] Fix address spaces on prvalue (PR #221233)

[Tom Honermann via cfe-commits]

================
@@ -0,0 +1,59 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+
+struct S {                                     // #ctors
+  S(int);                                      // #sint
+  void f() const;                              // #f-const
+  void f() __attribute__((address_space(1)));  // #f-as \
+                                               // expected-error {{function type may not be qualified with an address space}}
+};
+
+using const_int = const int;
+using as1_int = __attribute__((address_space(1))) int;
+using const_S = const S;
+using as1_S = __attribute__((address_space(1))) S;
+
+void testQualifiers() {
+  // Ok; const is dropped on prvalues of non-class type.
+  (void)(const_int{1});
+  // Ok; address space is dropped on prvalues of non-class type.
+  (void)(as1_int{1});
+  // Ok; const is retained on prvalues of class type; const qualified
+  // member function called.
+  const_S{1}.f();
+  // Error; address space is retained on prvalues of class type, but no
+  // constructor or member function can be called.
+  as1_S{1}.f();
+  // expected-error at -1 {{no matching constructor for initialization of 'as1_S' (aka '__attribute__((address_space(1))) S')}}
+  // expected-error at -2 {{no matching member function for call to 'f'}}
+  // expected-note@#ctors 2 {{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(1)))'}}
+  // expected-note@#sint {{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(1)))'}}
+  // expected-note@#f-const {{candidate function not viable: 'this' object is in address space '1', but method expects object in generic address space}}
+  // expected-note@#f-as {{candidate function not viable: 'this' object is in address space '1', but method expects object in generic address space}}
+}
+
+void temporaryMaterializationTest() {
+  // An address-space-qualified class temporary retains the address space on
+  // the materialized object, so no constructor can be used.
+  as1_S{0};
+  // expected-error at -1 {{no matching constructor for initialization of 'as1_S' (aka '__attribute__((address_space(1))) S')}}
+  // expected-note@#ctors 2 {{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(1)))'}}
+  // expected-note@#sint {{candidate constructor ignored: cannot be used to construct an object in address space '__attribute__((address_space(1)))'}}
+}
+
+// FIXME: All qualifiers including address space are retained on array elements
+//        The code in getNonLValueExprType() to remove qualifiers from prvalues
+//        acts on the array type and not the element. The code to remove address
+//        spaces is never hit. I am not sure this if this is correct behavior.
----------------
tahonermann wrote:

@elizabethandrews,
> I think we need to consider OpenCL attributes separately from `address_space(N)` attributes when studying how address spaces behave in C++, since the behavior varies.

I think the behavior is not intended to vary. The [`address_space` attribute documentation](https://clang.llvm.org/docs/AttributeReference.html#address-space), the [OpenCL C specification chapter 6.7, "Address Space Qualifiers"](https://registry.khronos.org/OpenCL/specs/unified/html/OpenCL_C.html#address-space-qualifiers), and the [C++ for OpenCL specification chapter 3.3, "Address spaces"](https://www.khronos.org/opencl/assets/CXX_for_OpenCL.html#address_space) all reference section 5 of [ISO/IEC TR 18037:2008](https://www.iso.org/obp/ui/#iso:std:iso-iec:tr:18037:ed-2:v1:en). The OpenCL specifications specify keywords as qualifiers, not attributes, but since Clang implements the keywords using attributes, I think use of the keyword or attribute spelling should exhibit the same behavior. Of course, ISO/IEC TR 18037 doesn't cover use in C++, so we're left to speculate what the behavior should be. The C++ for OpenCL specification specifies some behaviors for C++ (e.g., use of address space qualifiers as member function qualifiers), but not all.

With regard to your examples, I think we can take some inspiration from the 18037 specification. The C99 wording changes listed in section 5.3, "Detailed changes to ISO/IEC 9899:1999" include the following. From clause 6.5.2.5, "Compound literals":
> If the compound literal occurs inside the body of a function, the type name shall not be qualified by
> an address-space qualifier. 

>From clause 6.7.3, "Type qualifiers":
> The type of an object with automatic storage duration shall not be qualified by an address-space
> qualifier. 

C99 compound literals are effectively C's version of C++'s explicit temporary object creation, so it is reasonable to expect them to behave the same. Clang does correctly reject compound literals with an address space qualified type at local scope. For a double check, I compared behavior between GCC and Clang for a couple of address space qualifiers they both support (`__seg_fs`, `__seg_gs`) and found the behavior to match. See https://godbolt.org/z/5cxW4z4z4. Then, since those address spaces are not one of the ones we're really concerned with, I substituted `[[clang::address_space]]` for the keyword forms and verified behavior is consistent. See https://godbolt.org/z/bGn9noGE7.

I was really hoping to use GCC to compare behavior in C++ but, unfortunately, GCC doesn't provide the ISO 18037 address space extensions in C++ mode.

So back to your first example (https://godbolt.org/z/h6j3G9qeh). We're now in C++. The C++ standard is not particularly explicit in stating that temporary objects created at block scope have automatic storage duration, but they do. If we accept that, then I think your example is illustrating three interesting things.
1. An error is correctly issued for line 15.
2. Errors are correctly issued for lines 20 and 31, but for the wrong reason.
3. There is a missing error diagnostic for line 26 (the same error that should be issued for lines 20 and 31).

Lines 20, 26, and 31 all create a temporary object with an address space qualified type. Per above, each of those creations should be diagnosed.

The error that is issued at line 20 occurs because the temporary object (incorrectly constructed with an address space qualifier) is passed as the implicit object argument to `X::mf()` which has an implicit generic address space qualifier.

The error that is issued at line 31 occurs because the temporary array object (incorrectly constructed with an address space qualifier) element cannot be bound to a reference with a mismatched address space qualifier.

Your second example (https://godbolt.org/z/4dncr9xf3) demonstrates the same problem; Clang fails to diagnose the construction of the temporary objects which allows later errors to surface (that would otherwise have been suppressed since they depend on an expression with an error). Specifically, I think the aggregate initialization elides a constructor invocation, so the mismatched address space isn't caught until the call to `mf()` (which has an implicit generic address space qualifier). In the `Ctor` case, the constructor isn't elided, so the error is caught on call to the constructor.

> I am pretty confused with all of it now. How should https://godbolt.org/z/4dncr9xf3 actually behave? If prvalues don't have storage neither case should retain the address space right? This would however mean that for something like `as1_Ctor{1}.mf()` we would strip the address space like we do for scalars and it would silently construct a plain `Ctor`. This would mimic how scalars currently behave.

If Clang was handling the scalar case correctly, I think that would be right. But per above, I think the scalar case should be rejected just as it is for compound literals in C. I think the way to think about this is that the function cast expression (at block scope) produces an rvalue and then temporary materialization produces an xvalue. See https://godbolt.org/z/vTMfMqE7P. So there are two things we want to do:
1. Prohibit construction of an rvalue with a type that has an address space qualifier since temporary materialization (at block scope, with automatic storage duration) wouldn't be able to respect the address space.
2. Implicitly strip address space qualifiers only during lvalue-to-prvalue conversions which, I assume, is what `getNonLValueExprType()` does. 

I haven't looked at why `CXXFunctionalCastExpr()` routes through `getNonLValueExprType()` yet. It took me long enough to get this far and I'm tired now. More fun for tomorrow! :)

https://github.com/llvm/llvm-project/pull/221233