fix access checking about function overloading #107768

Backl1ght · 2024-09-08T16:37:07Z

After some more debugging, I find out that we will check access here at

llvm-project/clang/lib/Sema/SemaInit.cpp

Line 7807 in 8e010ac

S.CheckAddressOfMemberAccess(CurInit.get(), Step->Function.FoundDecl);

And for f() inside code below, Found.getAccess() is AS_none hence CheckAddressOfMemberAccess return AR_accessible directly.

struct Base {
public:
  int f(int);
private:
  int f();  // expect-note {{declared private here}}
};

struct Derived : public Base {};

void f() {
  int(Derived::* public_f)(int) = &Derived::f;
  int(Derived::* private_f)() = &Derived::f;  // expect-error {{'f' is a private member of 'Base'}}
}

I think the Found.getAccess() is intended to be AS_none so I just add one more access check for the UnresolvedLookupExpr when Found.getAccess() is AS_none. If add the check unconditionally clang will report lots of duplicate errors and cause several unit tests to fail.

I also test the UB mentioned in #107629 and clang now display 4 false as expecetd.

zyn0217 · 2024-09-30T06:59:42Z

What's the status here?

Backl1ght · 2024-10-11T15:25:33Z

Sorry for the late response, I will continue to look into this issue this sunday.

Backl1ght · 2024-10-13T14:53:55Z

After some more debugging, I find out that we will check access here at

llvm-project/clang/lib/Sema/SemaInit.cpp

Line 7807 in 8e010ac

S.CheckAddressOfMemberAccess(CurInit.get(), Step->Function.FoundDecl);

And for f() inside code below, Found.getAccess() is AS_none hence CheckAddressOfMemberAccess return AR_accessible directly.

struct Base {
public:
  int f(int);
private:
  int f();  // expect-note {{declared private here}}
};

struct Derived : public Base {};

void f() {
  int(Derived::* public_f)(int) = &Derived::f;
  int(Derived::* private_f)() = &Derived::f;  // expect-error {{'f' is a private member of 'Base'}}
}

llvmbot · 2024-12-01T09:00:16Z

@llvm/pr-subscribers-clang

Author: Zhikai Zeng (Backl1ght)

Changes

fix #107629

After some more debugging, I find out that we will check access here at

llvm-project/clang/lib/Sema/SemaInit.cpp

Line 7807 in 8e010ac

S.CheckAddressOfMemberAccess(CurInit.get(), Step->Function.FoundDecl);

And for f() inside code below, Found.getAccess() is AS_none hence CheckAddressOfMemberAccess return AR_accessible directly.

struct Base {
public:
  int f(int);
private:
  int f();  // expect-note {{declared private here}}
};

struct Derived : public Base {};

void f() {
  int(Derived::* public_f)(int) = &amp;Derived::f;
  int(Derived::* private_f)() = &amp;Derived::f;  // expect-error {{'f' is a private member of 'Base'}}
}

I think the Found.getAccess() is intended to be AS_none so I just add one more access check for the UnresolvedLookupExpr when Found.getAccess() is AS_none. If add the check unconditionally clang will report lots of duplicate errors and cause several unit tests to fail.

I also test the UB mentioned in #107629 and clang now display 4 false as expecetd.

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/lib/Sema/SemaOverload.cpp (+3)
(modified) clang/test/CXX/class.access/p4.cpp (+14-2)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e44aefa90ab386..0de249ced8309a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -763,6 +763,7 @@ Bug Fixes to C++ Support
 - Fixed a bug where bounds of partially expanded pack indexing expressions were checked too early. (#GH116105)
 - Fixed an assertion failure caused by using ``consteval`` in condition in consumed analyses. (#GH117385)
 - Fix a crash caused by incorrect argument position in merging deduced template arguments. (#GH113659)
+- Fix a bug where private access specifier of overloaded function not respected. (#GH107629)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 4c9e37bd286dee..35389957adf32d 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -16305,6 +16305,9 @@ ExprResult Sema::FixOverloadedFunctionReference(Expr *E, DeclAccessPair Found,
   }
 
   if (UnresolvedLookupExpr *ULE = dyn_cast<UnresolvedLookupExpr>(E)) {
+    if (Found.getAccess() == AS_none) {
+      CheckUnresolvedLookupAccess(ULE, Found);
+    }
     // FIXME: avoid copy.
     TemplateArgumentListInfo TemplateArgsBuffer, *TemplateArgs = nullptr;
     if (ULE->hasExplicitTemplateArgs()) {
diff --git a/clang/test/CXX/class.access/p4.cpp b/clang/test/CXX/class.access/p4.cpp
index ca98c9f90bd89e..6d4c8c004911db 100644
--- a/clang/test/CXX/class.access/p4.cpp
+++ b/clang/test/CXX/class.access/p4.cpp
@@ -21,11 +21,13 @@ namespace test0 {
   public:
     void foo(Public&);
   protected:
-    void foo(Protected&); // expected-note 2 {{declared protected here}}
+    void foo(Protected&); // expected-note 4 {{declared protected here}}
   private:
-    void foo(Private&); // expected-note 2 {{declared private here}}
+    void foo(Private&); // expected-note 4 {{declared private here}}
   };
 
+  class B : public A {};
+
   void test(A *op) {
     op->foo(PublicInst);
     op->foo(ProtectedInst); // expected-error {{'foo' is a protected member}}
@@ -35,6 +37,16 @@ namespace test0 {
     void (A::*b)(Protected&) = &A::foo; // expected-error {{'foo' is a protected member}}
     void (A::*c)(Private&) = &A::foo; // expected-error {{'foo' is a private member}}
   }
+
+  void test(B *op) {
+    op->foo(PublicInst);
+    op->foo(ProtectedInst); // expected-error {{'foo' is a protected member}}
+    op->foo(PrivateInst); // expected-error {{'foo' is a private member}}
+
+    void (B::*a)(Public&) = &B::foo;
+    void (B::*b)(Protected&) = &B::foo; // expected-error {{'foo' is a protected member}}
+    void (B::*c)(Private&) = &B::foo; // expected-error {{'foo' is a private member}}
+  }
 }
 
 // Member operators.

erichkeane

This seems reasonable to me? And passes tests?

The AS_none has 'different meanings in different contexts, which is confusing here, so I have no idea what it is supposed to mean here. But unless someone has a problem with this, I suspect we should just see what the fallout of this is...

THOUGH, because I'm not sure, perhaps we wait another ~week for the branch and only do this in Clang21+?

erichkeane · 2025-06-10T17:45:23Z

DID a merge commit to see if we can get CI to approve this one, then I'll merge it (or someone else feel free to if you see it green!).

fix llvm#107629 After some more debugging, I find out that we will check access here at https://github.com/llvm/llvm-project/blob/8e010ac5a173c9dee44b44324169a3e100a1a6fc/clang/lib/Sema/SemaInit.cpp#L7807 And for `f()` inside code below, `Found.getAccess()` is `AS_none` hence `CheckAddressOfMemberAccess` return `AR_accessible` directly. ```cpp struct Base { public: int f(int); private: int f(); // expect-note {{declared private here}} }; struct Derived : public Base {}; void f() { int(Derived::* public_f)(int) = &Derived::f; int(Derived::* private_f)() = &Derived::f; // expect-error {{'f' is a private member of 'Base'}} } ``` I think the `Found.getAccess()` is intended to be `AS_none` so I just add one more access check for the `UnresolvedLookupExpr` when `Found.getAccess()` is `AS_none`. If add the check unconditionally clang will report lots of duplicate errors and cause several unit tests to fail. I also test the UB mentioned in llvm#107629 and clang now display 4 `false` as expecetd. Co-authored-by: Erich Keane <[email protected]>

fix

694b7dd

Backl1ght force-pushed the overloading_access_check branch from 65172d8 to 694b7dd Compare December 1, 2024 08:45

Backl1ght requested review from Sirraide and zyn0217 December 1, 2024 08:59

Backl1ght marked this pull request as ready for review December 1, 2024 08:59

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 1, 2024

cor3ntin requested review from AaronBallman and erichkeane January 19, 2025 08:00

erichkeane approved these changes Jan 21, 2025

View reviewed changes

Merge branch 'main' into overloading_access_check

8035d2a

erichkeane merged commit 46b7a88 into llvm:main Jun 10, 2025
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix access checking about function overloading #107768

fix access checking about function overloading #107768

Uh oh!

Backl1ght commented Sep 8, 2024 •

edited

Loading

Uh oh!

zyn0217 commented Sep 30, 2024

Uh oh!

Backl1ght commented Oct 11, 2024

Uh oh!

Backl1ght commented Oct 13, 2024

Uh oh!

llvmbot commented Dec 1, 2024

Uh oh!

erichkeane left a comment

Uh oh!

erichkeane commented Jun 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix access checking about function overloading #107768

fix access checking about function overloading #107768

Uh oh!

Conversation

Backl1ght commented Sep 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zyn0217 commented Sep 30, 2024

Uh oh!

Backl1ght commented Oct 11, 2024

Uh oh!

Backl1ght commented Oct 13, 2024

Uh oh!

llvmbot commented Dec 1, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Jun 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Backl1ght commented Sep 8, 2024 •

edited

Loading