This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Take into account the current context when checking the accessibility of a member function pointer
ClosedPublic

Authored by ahatanak on Aug 18 2017, 10:27 PM.

Diff Detail

Repository
rC Clang

Event Timeline

ahatanak created this revision.Aug 18 2017, 10:27 PM
ahatanak updated this revision to Diff 111805.Aug 18 2017, 10:52 PM

Test access to protected member functions.

Rakete1111 added inline comments.
lib/Sema/SemaAccess.cpp
1798 ↗(On Diff #111805)

You don't need that scope resolution operator there. Also, I guess you don't have to create EC, you can just pass EffectiveContext(CurScope->getEntity()) directly to IsAccessible.

ahatanak updated this revision to Diff 118119.Oct 6 2017, 6:37 PM

Address review comments.

lib/Sema/SemaAccess.cpp
1798 ↗(On Diff #111805)

The scope resolution operator is needed here because IsAccessible's return type is AccessResult defined at the top of SemaAccess.cpp, not the one defined in Sema.h. If I remove the scope resolution operator, clang issues a warning ("comparison of two values with different enumeration types").

EffectiveContext is passed as a temporary now as you suggested.

arphaman accepted this revision.Feb 12 2018, 5:28 PM

LGTM, I didn't find any issues

This revision is now accepted and ready to land.Feb 12 2018, 5:28 PM
This revision was automatically updated to reflect the committed changes.
ahatanak reopened this revision.Mar 12 2018, 11:40 AM

The patch got reverted in r325335 as it broke the Chromium build. I'm reopening this review.

This revision is now accepted and ready to land.Mar 12 2018, 11:40 AM
ahatanak updated this revision to Diff 138066.Mar 12 2018, 12:00 PM

The new patch passes the calling context to CheckNonDependent so that Sema::CheckAddressOfMemberAccess uses the correct context to check accessibility of explicit template arguments. I initially tried moving the declaration of SavedContext in Sema::FinishTemplateArgumentDeduction to after the call to CheckNonDependent, but that caused test/SemaCXX/cxx1z-class-template-argument-deduction.cpp to fail. It seems like it changes the way lookup of default template arguments is done when the context (which is a CXXDeductionGuideDecl in the failing test case) is not set before ConvertDeducedTemplateArguments is called.

rnk resigned from this revision.Mar 26 2018, 5:42 PM

I don't see a new test case for the issue reduced out of Chromium. I'd recommend adding that.

I have to resign because I don't know enough about deduction to stamp this...

ahatanak updated this revision to Diff 148334.May 23 2018, 9:50 PM

Add a test case for the Chromium failure. Also, simplify a bit by capturing the calling context in Sema::DeduceTemplateArguments.

I don't particularly like that between setting the DeclContext (SemaTemplateDeduction.cpp:3814) and actually using it (CheckAccess() in SemaAccess.cpp:1459) are some 20 stack frames but it looks like you already tried fixing this "locally" in your initial approach.

I assume we can't get the appropriate DeclContext from Expr *OvlExpr in CheckAddressOfMemberAccess(), right?

It doesn't look like it's possible to get the context needed to do accessibility check (CXXMethodDecl 'method' for 'C::overloadedMethod' in the test case) from 'Expr *OvlExpr' in CheckAddressOfMemberAccess. It's possible to get the class in which the member is defined (class ''C' for C::overloadedMethod'), but we want the context in which the member is used (that is 'method').

jkorous accepted this revision.Jun 4 2018, 4:48 PM

LGTM.

Thank you for the explanation!

This revision was automatically updated to reflect the committed changes.