Extend ptr32 support to be applied on typedef

[Ariel-Burton]

Summary

Earlier, if the QualType was sugared, then we would error out
as it was not a pointer type, for example,

typedef int *int_star;

int_star __ptr32 p;

Now, if ptr32 is given we apply it if the raw Canonical Type
(i.e., the desugared type) is a PointerType, instead of only
checking whether the sugared type is a pointer type.

As before, we still disallow ptr32 usage if the pointer is used
as a pointer to a member.

[akhuang]

Looks good, thanks!

[rnk]

clang/lib/Sema/SemaType.cpp

7163

This seems like a good place to use getSingleStepDesugaredType to look through all type sugar (parens, typedefs, template substitutions, etc).

[Ariel-Burton]

clang/lib/Sema/SemaType.cpp

7163

This seems like a good place to use getSingleStepDesugaredType to look through all type sugar (parens, typedefs, template substitutions, etc).

I'm not sure what you mean. Could you expand a little, please?

[rnk]

clang/lib/Sema/SemaType.cpp

7163

Clang's AST has lots of "type sugar nodes". These are types which usually don't have any semantic meaning, they just carry source location information, like whether there was a typedef or extra parens in the type. AttributedType is also a type sugar node, so we cannot do a full desugaring here, we have to step through each node one at a time to accumulate the attributes.

Your code looks through one kind of type sugar, but this loop should probably be generalized to handle all kinds of type sugar. I think getSingleStepDesugaredType will do that.

[Ariel-Burton]

clang/lib/Sema/SemaType.cpp

7163

Thanks for the clarification. Do you mean something like:

for (;;) {
   const AttributedType *AT = dyn_cast<AttributedType>(Desugared);
   if (AT) {
     Attrs[AT->getAttrKind()] = true;
     Desugared = AT->getModifiedType();
   } else {
     QualType QT = Desugared.getSingleStepDesugaredType(S.Context);
     if (Desugared != QT) {
       Desugared = QT;
     } else {
       break;
     }
   }
 }

I don't think that we can use getSingleStepDesugaredType indiscriminately. Not all sugar should be peeled away, for example, in the case of of parentheses:

int *(__ptr32 wrong);

is accepted when it shouldn't.

To check for the cases where we don't want to desugar has the same sort of complexity as checking for the cases where we do. I suggest going with the original proposal.

[rnk]

That sounds reasonable to me, I confirmed that MSVC really only lets you apply these attributes directly to pointer types and to typedefs.

Can you add a test for the other most common type sugar node, the template parameter? It looks like this:

template <typename T>
void f(T __ptr32 a) {
    (*a) += 1;
}
void g(int *p) {
    f(p);
}

If there isn't already a C++ test for __ptr32 & co, go ahead and make one.

clang/lib/Sema/SemaType.cpp

7197

I raised the issue because it wasn't clear if we had a definitive answer for the question in this FIXME.

[Ariel-Burton]

In D130123#3691011, @rnk wrote:

That sounds reasonable to me, I confirmed that MSVC really only lets you apply these attributes directly to pointer types and to typedefs.

Can you add a test for the other most common type sugar node, the template parameter? It looks like this:

template <typename T>
void f(T __ptr32 a) {
    (*a) += 1;
}
void g(int *p) {
    f(p);
}

If there isn't already a C++ test for __ptr32 & co, go ahead and make one.

What is your expectation for your template code fragment? MSVC does not accept it.

On the other hand, MSVC does accept this:

template <typename T>
void f(T  a) {
  (*a) += sizeof(a);
}
void g(int *p) {
  f(p);
}
void h(int *__ptr32 p) {
    f(p);
}

[rnk]

In D130123#3696752, @Ariel-Burton wrote:

What is your expectation for your template code fragment? MSVC does not accept it.

Yes, I checked, MSVC rejects it, so clang should have test expectations to confirm that. It seems interesting or surprising, to me at least, that MSVC really only accepts __ptr32 on pointers and typedefs of them.

On the other hand, MSVC does accept this:

template <typename T>
void f(T  a) {
  (*a) += sizeof(a);
}
void g(int *p) {
  f(p);
}
void h(int *__ptr32 p) {
    f(p);
}

Right, this makes sense to me. MSVC's diagnostics say something about the __ptr32 qualifier needing to appear after a *, so this extension must be implemented at a pretty low-level, with some exception for typedefs, just like what you have.

[Ariel-Burton]

In D130123#3696786, @rnk wrote:

In D130123#3696752, @Ariel-Burton wrote:

What is your expectation for your template code fragment? MSVC does not accept it.

Yes, I checked, MSVC rejects it, so clang should have test expectations to confirm that. It seems interesting or surprising, to me at least, that MSVC really only accepts __ptr32 on pointers and typedefs of them.

On the other hand, MSVC does accept this:

template <typename T>
void f(T  a) {
  (*a) += sizeof(a);
}
void g(int *p) {
  f(p);
}
void h(int *__ptr32 p) {
    f(p);
}

Right, this makes sense to me. MSVC's diagnostics say something about the __ptr32 qualifier needing to appear after a *, so this extension must be implemented at a pretty low-level, with some exception for typedefs, just like what you have.

Thanks.

Just to make sure that we're on the same page, you'd like to see a test that confirms that clang is rejecting the template<typename T> void f(T __ptr32 a) example, and possibly one that checks that my example is accepted. Is that correct?

[rnk]

In D130123#3698399, @Ariel-Burton wrote:

Just to make sure that we're on the same page, you'd like to see a test that confirms that clang is rejecting the template<typename T> void f(T __ptr32 a) example, and possibly one that checks that my example is accepted. Is that correct?

Yep, exactly that. Sorry for the delay, the code looks good.

[Ariel-Burton]

Added C++ tests for __ptr32.

[Ariel-Burton]

Recover original changes, and commit with new C++ __ptr32 test.

[Ariel-Burton]

In D130123#3700174, @rnk wrote:

In D130123#3698399, @Ariel-Burton wrote:

Just to make sure that we're on the same page, you'd like to see a test that confirms that clang is rejecting the template<typename T> void f(T __ptr32 a) example, and possibly one that checks that my example is accepted. Is that correct?

Yep, exactly that. Sorry for the delay, the code looks good.

Please see updated test clang/test/Sema/MicrosoftExtensions.cpp

[rnk]

lgtm, thanks!

[Ariel-Burton]

• Add case to deal with ElaboratedTypes.

[rnk]

In D130123#3708309, @Ariel-Burton wrote:

• Add case to deal with ElaboratedTypes.

Let's still stick with this code, but I at least feel justified raising the concern that perhaps we should look through other kinds of type sugar. :)

clang/lib/Sema/SemaType.cpp

case ParsedAttr::AT_SPtr:
     A = createSimpleAttr<SPtrAttr>(S.Context, PAttr);
     break;
   case ParsedAttr::AT_UPtr:
     A = createSimpleAttr<UPtrAttr>(S.Context, PAttr);
     break;
   }
 
   std::bitset<attr::LastAttr> Attrs;
-  attr::Kind NewAttrKind = A->getKind();
   QualType Desugared = Type;
-  const AttributedType *AT = dyn_cast<AttributedType>(Type);
-  while (AT) {
+  for (;;) {
+    if (const TypedefType *TT = dyn_cast<TypedefType>(Desugared)) {
+      Desugared = TT->desugar();
+      continue;
+    } else if (const ElaboratedType *ET = dyn_cast<ElaboratedType>(Desugared)) {
+      Desugared = ET->desugar();
+      continue;
+    }
+    const AttributedType *AT = dyn_cast<AttributedType>(Desugared);
+    if (!AT)
+      break;
     Attrs[AT->getAttrKind()] = true;
     Desugared = AT->getModifiedType();
-    AT = dyn_cast<AttributedType>(Desugared);
   }
 
   // You cannot specify duplicate type attributes, so if the attribute has
   // already been applied, flag it.
+  attr::Kind NewAttrKind = A->getKind();
   if (Attrs[NewAttrKind]) {
     S.Diag(PAttr.getLoc(), diag::warn_duplicate_attribute_exact) << PAttr;
     return true;
   }
   Attrs[NewAttrKind] = true;
 
   // You cannot have both __sptr and __uptr on the same type, nor can you
   // have __ptr32 and __ptr64.
   if (Attrs[attr::Ptr32] && Attrs[attr::Ptr64]) {
     S.Diag(PAttr.getLoc(), diag::err_attributes_are_not_compatible)
         << "'__ptr32'"
         << "'__ptr64'";
     return true;
   } else if (Attrs[attr::SPtr] && Attrs[attr::UPtr]) {
     S.Diag(PAttr.getLoc(), diag::err_attributes_are_not_compatible)
         << "'__sptr'"
         << "'__uptr'";
     return true;
   }
 
+  // Check the raw (i.e., desugared) Canonical type to see if it
+  // is a pointer type.
+  if (!isa<PointerType>(Desugared)) {
     // Pointer type qualifiers can only operate on pointer types, but not
     // pointer-to-member types.
-  //
-  // FIXME: Should we really be disallowing this attribute if there is any
-  // type sugar between it and the pointer (other than attributes)? Eg, this
-  // disallows the attribute on a parenthesized pointer.
-  // And if so, should we really allow *any* type attribute?
-  if (!isa<PointerType>(Desugared)) {
     if (Type->isMemberPointerType())
       S.Diag(PAttr.getLoc(), diag::err_attribute_no_member_pointers) << PAttr;
     else
       S.Diag(PAttr.getLoc(), diag::err_attribute_pointers_only) << PAttr << 0;
     return true;
   }
 
   // Add address space to type based on its attributes.

clang/test/CodeGen/address-space-ptr32.c

 // RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-windows-msvc -fms-extensions -emit-llvm < %s | FileCheck %s
 
+_Static_assert(sizeof(void *) == 8, "sizeof(void *) has unexpected value.  Expected 8.");
+
 int foo(void) {
+  // CHECK: define dso_local i32 @foo
+  // CHECK: %a = alloca i32 (i32) addrspace(270)*, align 4
+  // CHECK: ret i32 4
   int (*__ptr32 a)(int);
   return sizeof(a);
 }
 
-// CHECK: define dso_local i32 @foo
-// CHECK: %a = alloca i32 (i32) addrspace(270)*, align 4
+int bar(void) {
+  // CHECK: define dso_local i32 @bar
+  // CHECK: %p = alloca i32 addrspace(270)*, align 4
+  // CHECK: ret i32 4
+  int *__ptr32 p;
+  return sizeof(p);
+}
+
+
+int baz(void) {
+  // CHECK: define dso_local i32 @baz
+  // CHECK: %p = alloca i32 addrspace(270)*, align 4
+  // CHECK: ret i32 4
+  typedef int *__ptr32 IP32_PTR;
+
+  IP32_PTR p;
+  return sizeof(p);
+}
+
+int fugu(void) {
+  // CHECK: define dso_local i32 @fugu
+  // CHECK: %p = alloca i32 addrspace(270)*, align 4
   // CHECK: ret i32 4
+  typedef int *int_star;
+
+  int_star __ptr32 p;
+  return sizeof(p);
+}

clang/test/Sema/MicrosoftExtensions.c

 int *wrong6 __ptr32;  // expected-error {{expected ';' after top level declarator}} expected-warning {{declaration does not declare anything}}
 
 int * __ptr32 __ptr64 wrong7;  // expected-error {{'__ptr32' and '__ptr64' attributes are not compatible}}
 
 int * __ptr32 __ptr32 wrong8;  // expected-warning {{attribute '__ptr32' is already applied}}
 
 int *(__ptr32 __sptr wrong9); // expected-error {{'__sptr' attribute only applies to pointer arguments}} // expected-error {{'__ptr32' attribute only applies to pointer arguments}}
 
+int *(__ptr32 wrong10); // expected-error {{'__ptr32' attribute only applies to pointer arguments}}
+
+int *(__ptr64 wrong11); // expected-error {{'__ptr64' attribute only applies to pointer arguments}}
+
+int *(__ptr32 __ptr64 wrong12); // expected-error {{'__ptr32' attribute only applies to pointer arguments}} // expected-error {{'__ptr64' attribute only applies to pointer arguments}}
+
 typedef int *T;
-T __ptr32 wrong10; // expected-error {{'__ptr32' attribute only applies to pointer arguments}}
+T __ptr32 ok1;
+T __ptr64 ok2;
+T __ptr32 __ptr64 wrong13; // expected-error {{'__ptr32' and '__ptr64' attributes are not compatible}}
+
+typedef int *__ptr32 T1;
+T1 ok3;
+T1 __ptr32 wrong14;  // expected-warning {{attribute '__ptr32' is already applied}}
+T1 __ptr64 wrong15;  // expected-error {{'__ptr32' and '__ptr64' attributes are not compatible}}
+
+typedef int *__ptr64 T2;
+T2 ok4;
+T2 __ptr64 wrong16;  // expected-warning {{attribute '__ptr64' is already applied}}
+T2 __ptr32 wrong17;  // expected-error {{'__ptr32' and '__ptr64' attributes are not compatible}}
+
+typedef int *__ptr32 __ptr64 wrong18; // expected-error {{'__ptr32' and '__ptr64' attributes are not compatible}}
 
 typedef char *my_va_list;
 void __va_start(my_va_list *ap, ...); // expected-note {{passing argument to parameter 'ap' here}}
 void vmyprintf(const char *f, my_va_list ap);
 void myprintf(const char *f, ...) {
   my_va_list ap;
   if (1) {
     __va_start(&ap, f);

clang/test/Sema/MicrosoftExtensions.cpp

 // RUN: %clang_cc1 -triple i686-windows %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
 // RUN: %clang_cc1 -triple x86_64-windows %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
-// expected-no-diagnostics
 
 // Check that __ptr32/__ptr64 can be compared.
 int test_ptr_comparison(int *__ptr32 __uptr p32u, int *__ptr32 __sptr p32s,
                         int *__ptr64 p64) {
   return (p32u == p32s) +
          (p32u == p64) +
          (p32s == p64);
 }
+
+template<typename T>
+void bad(T __ptr32 a) { // expected-error {{'__ptr32' attribute only applies to pointer arguments}}`
+  (*a) += 1;
+}
+
+template<int size_expected, typename T>
+void f(T a) {
+  (*a) += sizeof(a);
+  static_assert(sizeof(a) == size_expected, "instantiated template argument has unexpected size");
+}
+void g(int *p) {
+  // instantiate for default sized pointer
+  f<sizeof(void*)>(p);
+}
+
+void h(int *__ptr32 p) {
+  // instantiate for 32-bit pointer
+  f<4>(p);
+}