[Clang] [Sema] Handle this in __restrict-qualified member functions properly

[Sirraide]

When resolving the type of this inside a member function, we were attaching all qualifiers present on the member function to the class type and then making it a pointer; however, __restrict, unlike const and volatile, needs to be attached to the pointer type rather than the pointee type.

This fixes #82941, #42411, and #18121.

[Sirraide]

CC @shafik, @AaronBallman

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (Sirraide)

Changes

When resolving the type of this inside a member function, we were attaching all qualifiers present on the member function to the class type and then making it a pointer; however, __restrict, unlike const and volatile, needs to be attached to the pointer type rather than the pointee type.

This fixes #82941.

---

Full diff: https://github.com/llvm/llvm-project/pull/83187.diff

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/lib/AST/DeclCXX.cpp (+13-2)
• (modified) clang/lib/Sema/SemaExprCXX.cpp (+1-1)
• (added) clang/test/SemaCXX/restrict-this.cpp (+51)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7e16b9f0c67dbd..d420f572a84487 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -290,6 +290,9 @@ Bug Fixes to C++ Support
   lookup searches the bases of an incomplete class.
 - Fix a crash when an unresolved overload set is encountered on the RHS of a ``.*`` operator.
   (`#53815 <https://github.com/llvm/llvm-project/issues/53815>`_)
+- In ``__restrict``-qualified member functions, attach ``__restrict`` to the pointer type of
+  ``this`` rather than the pointee type.
+  Fixes (`#82941 <https://github.com/llvm/llvm-project/issues/82941>`_).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 117e802dae2d9d..b4f2327d9c560a 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -2543,8 +2543,19 @@ QualType CXXMethodDecl::getThisType(const FunctionProtoType *FPT,
                                     const CXXRecordDecl *Decl) {
   ASTContext &C = Decl->getASTContext();
   QualType ObjectTy = ::getThisObjectType(C, FPT, Decl);
-  return C.getLangOpts().HLSL ? C.getLValueReferenceType(ObjectTy)
-                              : C.getPointerType(ObjectTy);
+
+  // Unlike 'const' and 'volatile', a '__restrict' qualifier must be
+  // attached to the pointer type, not the pointee.
+  bool Restrict = FPT->getMethodQuals().hasRestrict();
+  if (Restrict)
+    ObjectTy.removeLocalRestrict();
+
+  ObjectTy = C.getLangOpts().HLSL ? C.getLValueReferenceType(ObjectTy)
+                                  : C.getPointerType(ObjectTy);
+
+  if (Restrict)
+    ObjectTy.addRestrict();
+  return ObjectTy;
 }
 
 QualType CXXMethodDecl::getThisType() const {
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 59758d3bd6d1a3..c4750ce78fa9c1 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1220,7 +1220,7 @@ static QualType adjustCVQualifiersForCXXThisWithinLambda(
                     : nullptr;
     }
   }
-  return ASTCtx.getPointerType(ClassType);
+  return ThisTy;
 }
 
 QualType Sema::getCurrentThisType() {
diff --git a/clang/test/SemaCXX/restrict-this.cpp b/clang/test/SemaCXX/restrict-this.cpp
new file mode 100644
index 00000000000000..8cd89f0f36a76c
--- /dev/null
+++ b/clang/test/SemaCXX/restrict-this.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+// expected-no-diagnostics
+
+struct C {
+  void f() __restrict {
+    static_assert(__is_same(decltype(this), C *__restrict));
+    (void) [this]() {
+      static_assert(__is_same(decltype(this), C *__restrict));
+      (void) [this]() { static_assert(__is_same(decltype(this), C *__restrict)); };
+
+      // By-value capture means 'this' is now a different object; do not
+      // make it __restrict.
+      (void) [*this]() { static_assert(__is_same(decltype(this), const C *)); };
+      (void) [*this]() mutable { static_assert(__is_same(decltype(this), C *)); };
+    };
+  }
+};
+
+template <typename T> struct TC {
+  void f() __restrict {
+    static_assert(__is_same(decltype(this), TC<int> *__restrict));
+    (void) [this]() {
+      static_assert(__is_same(decltype(this), TC<int> *__restrict));
+      (void) [this]() { static_assert(__is_same(decltype(this), TC<int> *__restrict)); };
+
+      // By-value capture means 'this' is now a different object; do not
+      // make it __restrict.
+      (void) [*this]() { static_assert(__is_same(decltype(this), const TC<int> *)); };
+      (void) [*this]() mutable { static_assert(__is_same(decltype(this), TC<int> *)); };
+    };
+  }
+};
+
+void f() {
+  TC<int>{}.f();
+}
+
+namespace gh82941 {
+void f(int& x) {
+    (void)x;
+}
+
+class C {
+    int x;
+    void g() __restrict;
+};
+
+void C::g() __restrict {
+    f(this->x);
+}
+}

[AaronBallman]

Thank you for the fix! This seems to fix quite a few issues because it also addresses:

#42411
#18121
#11039

(which should be added to test cases if there's something unique there, and mentioned in the release notes and commit message).

Otherwise, LGTM!

[Sirraide]

Thank you for the fix! This seems to fix quite a few issues because it also addresses:

#42411 #18121 #11039

I also checked for other open issues, but I see you were more successful than me in finding some; I’ll take a look at those and add tests for them as well.

[Sirraide]

#11039

It does not fix this one because whereas GCC supports this, we don’t:

struct foo {
    void f();
};

void foo::f() __restrict {

}

because __restrict here is treated like const, so it has to be present on both the declaration and definition; however, GCC accepts this; should we do so as well?

The other two issues seem to be fixed by this, though.

[Sirraide]

however, GCC accepts this; should we do so as well?

If so, should that be a separate pr, perhaps?

[Sirraide]

however, GCC accepts this; should we do so as well?

If so, should that be a separate pr, perhaps?

@AaronBallman If you think it’s related enough I can look into this as well for this pr; otherwise, I’m gonna go ahead and merge this one and then probably look into that next because it would make sense to support that imo if it’s not too complicated to implement.

[AaronBallman]

LGTM! I think the other work should be done in a follow-up patch as it is slightly different than this fix.