[Clang][Sema] Handle class member access expressions with valid nested-name-specifiers that become invalid after lookup by sdkrystian · Pull Request #98167 · llvm/llvm-project

sdkrystian · 2024-07-09T15:12:44Z

The following code causes this assert to fail:

struct A { };
struct B;

void f(A *x) {
  x->B::y; // crash here
}

This happens because we only return early from BuildMemberReferenceExpr when the CXXScopeSpecifier is invalid before the lookup is performed. Since the lookup may invalidate the CXXScopeSpecifier (e.g. if the nested-name-specifier is incomplete), this results in the second BuildMemberReferenceExpr overload being called with an invalid CXXScopeSpecifier, which causes the assert to fail. This patch moves the early return for invalid CXXScopeSpecifiers to occur after lookup is performed. This fixes #92972.

I also removed the if (SS.isSet() && SS.isInvalid()) check in ActOnMemberAccessExpr because the condition can never be true (isSet returns getScopeRep() != nullptr and isInvalid returns Range.isValid() && getScopeRep() == nullptr).

llvmbot · 2024-07-09T15:13:28Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

The following code causes this assert to fail:

struct A { };
struct B;

void f(A *x) {
  x-&gt;B::y; // crash here
}

This happens because we only return early from BuildMemberReferenceExpr when the CXXScopeSpecifier is invalid before the lookup is performed. Since the lookup may invalidate the CXXScopeSpecifier (e.g. if the nested-name-specifier is incomplete), this results in the second BuildMemberReferenceExpr overload to be called with an invalid CXXScopeSpecifier, which causes the assert to fail. This patch moves the early return for invalid CXXScopeSpecifiers to occur after lookup is performed. This fixes #92972.

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

2 Files Affected:

(modified) clang/lib/Sema/SemaExprMember.cpp (+6-10)
(added) clang/test/CXX/basic/basic.lookup/basic.lookup.qual/basic.lookup.qual.general/p2.cpp (+16)

diff --git a/clang/lib/Sema/SemaExprMember.cpp b/clang/lib/Sema/SemaExprMember.cpp
index b7ea24790d361..daa9a4948f66c 100644
--- a/clang/lib/Sema/SemaExprMember.cpp
+++ b/clang/lib/Sema/SemaExprMember.cpp
@@ -789,9 +789,6 @@ ExprResult Sema::BuildMemberReferenceExpr(
     ActOnMemberAccessExtraArgs *ExtraArgs) {
   LookupResult R(*this, NameInfo, LookupMemberName);
 
-  if (SS.isInvalid())
-    return ExprError();
-
   // Implicit member accesses.
   if (!Base) {
     TypoExpr *TE = nullptr;
@@ -826,6 +823,10 @@ ExprResult Sema::BuildMemberReferenceExpr(
     BaseType = Base->getType();
   }
 
+  // BuildMemberReferenceExpr expects a valid nested-name-specifier, if any.
+  if (SS.isInvalid())
+    return ExprError();
+
   return BuildMemberReferenceExpr(Base, BaseType,
                                   OpLoc, IsArrow, SS, TemplateKWLoc,
                                   FirstQualifierInScope, R, TemplateArgs, S,
@@ -1745,14 +1746,9 @@ static ExprResult LookupMemberExpr(Sema &S, LookupResult &R,
 
 ExprResult Sema::ActOnMemberAccessExpr(Scope *S, Expr *Base,
                                        SourceLocation OpLoc,
-                                       tok::TokenKind OpKind,
-                                       CXXScopeSpec &SS,
+                                       tok::TokenKind OpKind, CXXScopeSpec &SS,
                                        SourceLocation TemplateKWLoc,
-                                       UnqualifiedId &Id,
-                                       Decl *ObjCImpDecl) {
-  if (SS.isSet() && SS.isInvalid())
-    return ExprError();
-
+                                       UnqualifiedId &Id, Decl *ObjCImpDecl) {
   // Warn about the explicit constructor calls Microsoft extension.
   if (getLangOpts().MicrosoftExt &&
       Id.getKind() == UnqualifiedIdKind::IK_ConstructorName)
diff --git a/clang/test/CXX/basic/basic.lookup/basic.lookup.qual/basic.lookup.qual.general/p2.cpp b/clang/test/CXX/basic/basic.lookup/basic.lookup.qual/basic.lookup.qual.general/p2.cpp
new file mode 100644
index 0000000000000..ebdae971a929e
--- /dev/null
+++ b/clang/test/CXX/basic/basic.lookup/basic.lookup.qual/basic.lookup.qual.general/p2.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -verify -Wno-unused %s
+
+struct A {
+  int y;
+};
+
+struct B; // expected-note 4{{forward declaration of 'B'}}
+
+void f(A *a, B *b) {
+  a->B::x; // expected-error {{incomplete type 'B' named in nested name specifier}}
+  a->A::x; // expected-error {{no member named 'x' in 'A'}}
+  a->A::y;
+  b->B::x; // expected-error {{member access into incomplete type 'B'}}
+  b->A::x; // expected-error {{member access into incomplete type 'B'}}
+  b->A::y; // expected-error {{member access into incomplete type 'B'}}
+}

…d-name-specifiers that become invalid after lookup

cor3ntin

LGTM but can you add a changelog entry?

ilovepi · 2024-07-09T15:21:26Z

Thanks for running this down!

sdkrystian · 2024-07-09T15:23:10Z

@cor3ntin This fixes a bug introduced in this release, so I don't believe a release note is needed.

Reverts: more Sema changes 9e1f1cf [Clang][Sema] Handle class member access expressions with valid nested-name-specifiers that become invalid after lookup (llvm#98167) Change-Id: I80aa10cf856f4854120e2eb7446335403b163b77

…d-name-specifiers that become invalid after lookup (llvm#98167) The following code causes an assert in `SemaExprMember.cpp` on line 981 to fail: ``` struct A { }; struct B; void f(A *x) { x->B::y; // crash here } ``` This happens because we only return early from `BuildMemberReferenceExpr` when the `CXXScopeSpecifier` is invalid _before_ the lookup is performed. Since the lookup may invalidate the `CXXScopeSpecifier` (e.g. if the _nested-name-specifier_ is incomplete), this results in the second `BuildMemberReferenceExpr` overload being called with an invalid `CXXScopeSpecifier`, which causes the assert to fail. This patch moves the early return for invalid `CXXScopeSpecifiers` to occur _after_ lookup is performed. This fixes llvm#92972. I also removed the `if (SS.isSet() && SS.isInvalid())` check in `ActOnMemberAccessExpr` because the condition can never be true (`isSet` returns `getScopeRep() != nullptr` and `isInvalid` returns `Range.isValid() && getScopeRep() == nullptr`).

cherry-picked: 9e1f1cf sdkrystian@gmail.com Tue Jul 9 16:40:53 2024 -0400 [Clang][Sema] Handle class member access expressions with valid nested-name-specifiers that become invalid after lookup (llvm#98167) 584e431 sdkrystian@gmail.com Wed Jul 3 12:12:53 2024 -0400 [Clang][Sema] Treat explicit specializations of static data member templates declared without 'static' as static data members when diagnosing uses of 'auto' (llvm#97425) a1bce0b dblaikie@gmail.com Thu Jun 27 08:17:40 2024 -0700 Clang: Add warning flag for storage class specifiers on explicit specializations (llvm#96699) f46d146 erickvelez7@gmail.com Fri May 31 11:02:21 2024 -0700 [clang] require template arg list after template kw (llvm#80801) 033ec09 hanwei62@huawei.com Fri Dec 22 09:00:41 2023 +0800 [Clang][Sema] Fix Wswitch-default bad warning in template (llvm#76007) c281782 dongjianqiang2@huawei.com Thu Dec 7 09:03:15 2023 +0800 [clang][Sema] Add -Wswitch-default warning option (llvm#73077) Change-Id: Ib2582201b01cc62c3bf62011347925556e8531f7

sdkrystian requested review from AaronBallman and mizvekov July 9, 2024 15:12

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

[Clang][Sema] Handle class member access expressions with valid neste…

57b79eb

…d-name-specifiers that become invalid after lookup

sdkrystian force-pushed the member-expr-invalid branch from 85efbad to 57b79eb Compare July 9, 2024 15:14

sdkrystian mentioned this pull request Jul 9, 2024

Clang-Doc hits an assertion in Sema when run on LLVM #92972

Closed

cor3ntin approved these changes Jul 9, 2024

View reviewed changes

mizvekov approved these changes Jul 9, 2024

View reviewed changes

sdkrystian merged commit 9e1f1cf into llvm:main Jul 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] Handle class member access expressions with valid nested-name-specifiers that become invalid after lookup#98167

[Clang][Sema] Handle class member access expressions with valid nested-name-specifiers that become invalid after lookup#98167
sdkrystian merged 1 commit intollvm:mainfrom
sdkrystian:member-expr-invalid

sdkrystian commented Jul 9, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jul 9, 2024

Uh oh!

cor3ntin left a comment

Uh oh!

ilovepi commented Jul 9, 2024

Uh oh!

sdkrystian commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

sdkrystian commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 9, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

ilovepi commented Jul 9, 2024

Uh oh!

sdkrystian commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sdkrystian commented Jul 9, 2024 •

edited

Loading