[Sema] Allow comparisons between different ms ptr size address space types.

[akhuang]

Summary

We're currently using address spaces to implement ptr32/ptr64 attributes;
this patch fixes a bug where clang doesn't allow types with different pointer
size attributes to be compared.

Fixes https://bugs.llvm.org/show_bug.cgi?id=51889

[aaron.ballman]

clang/lib/Sema/SemaExprCXX.cpp

6682

I'm pretty sure this is correct based on my inspection of what code MSVC is generating. But it would be helpful to have some codegen tests in Clang for this functionality as well.

clang/test/Sema/MicrosoftExtensions.cpp

10–11

(Side question, not directly about this patch but sorta related.) Should there be a diagnostic about conversion potentially causing a possible loss of data (on the 64-bit target)?

[akhuang]

clang/lib/Sema/SemaExprCXX.cpp

6682

ha, I apparently didn't check that the behavior actually matches.. apparently in MSVC a ptr32 isn't equivalent to a ptr64

[aaron.ballman]

clang/lib/Sema/SemaExprCXX.cpp

6682

Oh! I had tested this:

int test1(int * __ptr32 __uptr p32u, int * __ptr32 __sptr p32s,
                         int * __ptr64 p64) {
  return (p32u == p64);
}

int test2(int * __ptr32 __uptr p32u, int * __ptr32 __sptr p32s,
                         int * __ptr64 p64) {
  return (p64 == p32u);
}

to see whether the order of the operands mattered as to which conversion "won" and I thought I saw that your patch generates the same code that MSVC does. However, I could have messed my testing up somehow, so double-checking is a good idea.

[akhuang]

clang/lib/Sema/SemaExprCXX.cpp

6682

Whoops, you're right, I was not specifying sptr/uptr and I guess clang and msvc have different defaults for sign/zero extension.

Will add tests. Also, thanks for reviewing!

clang/test/Sema/MicrosoftExtensions.cpp

10–11

Hmm, it seems reasonable, but I also don't know how motivated I am to add a diagnostic --

[aaron.ballman]

clang/test/Sema/MicrosoftExtensions.cpp

10–11

I don't insist -- we are missing that warning in general here (we don't warn on assignment yet). But it might be a nice follow-up for anyone who's interested in working on it.

[akhuang]

add test

[aaron.ballman]

LGTM, thank you!

clang/lib/Sema/SemaExprCXX.cpp

if (!Steps.empty()) {
 
       // Under one level of pointer or pointer-to-member, we can change to an
       // unambiguous compatible address space.
       if (Q1.getAddressSpace() == Q2.getAddressSpace()) {
         Quals.setAddressSpace(Q1.getAddressSpace());
       } else if (Steps.size() == 1) {
         bool MaybeQ1 = Q1.isAddressSpaceSupersetOf(Q2);
         bool MaybeQ2 = Q2.isAddressSpaceSupersetOf(Q1);
-        if (MaybeQ1 == MaybeQ2)
+        if (MaybeQ1 == MaybeQ2) {
+          // Exception for ptr size address spaces. Should be able to choose
+          // either address space during comparison.
+          if (isPtrSizeAddressSpace(Q1.getAddressSpace()) ||
+              isPtrSizeAddressSpace(Q2.getAddressSpace()))
+            MaybeQ1 = true;
+          else
             return QualType(); // No unique best address space.
+        }
         Quals.setAddressSpace(MaybeQ1 ? Q1.getAddressSpace()
                                       : Q2.getAddressSpace());
       } else {
         return QualType();
       }
 
       // FIXME: In C, we merge __strong and none to __strong at the top level.
       if (Q1.getObjCGCAttr() == Q2.getObjCGCAttr())

clang/test/CodeGen/ms-mixed-ptr-sizes.c

-// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -emit-llvm -O2 < %s | FileCheck %s --check-prefix=X64
-// RUN: %clang_cc1 -triple i386-pc-win32 -fms-extensions -emit-llvm -O2 < %s | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -emit-llvm -O2 < %s | FileCheck %s --check-prefixes=X64,ALL
+// RUN: %clang_cc1 -triple i386-pc-win32 -fms-extensions -emit-llvm -O2 < %s | FileCheck %s --check-prefixes=X86,ALL
 
 struct Foo {
   int * __ptr32 p32;
   int * __ptr64 p64;
 };
 void use_foo(struct Foo *f);
 void test_sign_ext(struct Foo *f, int * __ptr32 __sptr i) {
 // X64-LABEL: define dso_local void @test_sign_ext({{.*}}i32 addrspace(270)* %i)

 void test_other(struct Foo *f, __attribute__((address_space(10))) int *i) {
 // X64-LABEL: define dso_local void @test_other({{.*}}i32 addrspace(10)* %i)
 // X86-LABEL: define dso_local void @test_other({{.*}}i32 addrspace(10)* %i)
 // X64: %{{.+}} = addrspacecast i32 addrspace(10)* %i to i32 addrspace(270)*
 // X86: %{{.+}} = addrspacecast i32 addrspace(10)* %i to i32*
   f->p32 = (int * __ptr32)i;
   use_foo(f);
 }
+
+int test_compare1(int *__ptr32 __uptr i, int *__ptr64 j) {
+  // ALL-LABEL: define dso_local i32 @test_compare1
+  // X64: %{{.+}} = addrspacecast i32* %j to i32 addrspace(271)*
+  // X64: %cmp = icmp eq i32 addrspace(271)* %{{.+}}, %i
+  // X86: %{{.+}} = addrspacecast i32 addrspace(272)* %j to i32 addrspace(271)*
+  // X86: %cmp = icmp eq i32 addrspace(271)* %{{.+}}, %i
+  return (i == j);
+}
+
+int test_compare2(int *__ptr32 __sptr i, int *__ptr64 j) {
+  // ALL-LABEL: define dso_local i32 @test_compare2
+  // X64: %{{.+}} = addrspacecast i32* %j to i32 addrspace(270)*
+  // X64: %cmp = icmp eq i32 addrspace(270)* %{{.+}}, %i
+  // X86: %{{.+}} = addrspacecast i32 addrspace(272)* %j to i32*
+  // X86: %cmp = icmp eq i32* %{{.+}}, %i
+  return (i == j);
+}
+
+int test_compare3(int *__ptr32 __uptr i, int *__ptr64 j) {
+  // ALL-LABEL: define dso_local i32 @test_compare3
+  // X64: %{{.+}} = addrspacecast i32 addrspace(271)* %i to i32*
+  // X64: %cmp = icmp eq i32* %{{.+}}, %j
+  // X86: %{{.+}} = addrspacecast i32 addrspace(271)* %i to i32 addrspace(272)*
+  // X86: %cmp = icmp eq i32 addrspace(272)* %{{.+}}, %j
+  return (j == i);
+}
+
+int test_compare4(int *__ptr32 __sptr i, int *__ptr64 j) {
+  // ALL-LABEL: define dso_local i32 @test_compare4
+  // X64: %{{.+}} = addrspacecast i32 addrspace(270)* %i to i32*
+  // X64: %cmp = icmp eq i32* %{{.+}}, %j
+  // X86: %{{.+}} = addrspacecast i32* %i to i32 addrspace(272)*
+  // X86: %cmp = icmp eq i32 addrspace(272)* %{{.+}}, %j
+  return (j == i);
+}

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