[Clang][Sema] Fix crash when diagnosing near-match for 'constexpr' redeclaration in C++11 #92452

sdkrystian · 2024-05-16T20:51:51Z

Clang crashes when diagnosing the following invalid redeclaration in C++11:

struct A {
  void f();
};

constexpr void A::f() { } // crash here

This happens because DiagnoseInvalidRedeclaration tries to create a fix-it to remove const from the out-of-line declaration of f, but there is no SourceLocation for the const qualifier (it's implicitly const due to constexpr) and an assert in FunctionTypeInfo::getConstQualifierLoc fails.

This patch changes the accessors for cv-qualifier-seq SourceLocations in FunctionTypeInfo to return an invalid SourceLocation if no cv-qualifier-seq is present, and changes DiagnoseInvalidRedeclaration to only suggest the removal of the const qualifier when it was explicitly specified.

llvmbot · 2024-05-16T20:52:19Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Clang crashes when diagnosing the following invalid redeclaration in C++11:

struct A {
  void f();
};

constexpr void A::f() { } // crash here

This happens because DiagnoseInvalidRedeclaration tries to create a fix-it to remove const from the out-of-line declaration of f, but there is no SourceLocation for the const qualifier (it's implicitly const due to constexpr) and an assert in FunctionTypeInfo::getConstQualifierLoc fails.

This patch changes the accessors for cv-qualifier-seq SourceLocations in FunctionTypeInfo to return an invalid SourceLocation if no cv-qualifier-seq is present, and changes DiagnoseInvalidRedeclaration to only suggest the removal of the const qualifier when it was explicitly specified.

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

4 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/include/clang/Sema/DeclSpec.h (+6-6)
(modified) clang/lib/Sema/SemaDecl.cpp (+9-9)
(modified) clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p1.cpp (+11)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2d2928e418623..411a5f752899d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -724,6 +724,8 @@ Bug Fixes to C++ Support
   templates during partial ordering when deducing template arguments from a function declaration or when
   taking the address of a function template.
 - Fix a bug with checking constrained non-type template parameters for equivalence. Fixes (#GH77377).
+- Fix a C++11 crash when a non-const non-static member function is defined out-of-line with
+  the ``constexpr`` specifier.
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/DeclSpec.h b/clang/include/clang/Sema/DeclSpec.h
index 23bc780e04979..44d96db54b5f0 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -1527,20 +1527,20 @@ struct DeclaratorChunk {
 
     /// Retrieve the location of the 'const' qualifier.
     SourceLocation getConstQualifierLoc() const {
-      assert(MethodQualifiers);
-      return MethodQualifiers->getConstSpecLoc();
+      return MethodQualifiers ? MethodQualifiers->getConstSpecLoc()
+                              : SourceLocation();
     }
 
     /// Retrieve the location of the 'volatile' qualifier.
     SourceLocation getVolatileQualifierLoc() const {
-      assert(MethodQualifiers);
-      return MethodQualifiers->getVolatileSpecLoc();
+      return MethodQualifiers ? MethodQualifiers->getVolatileSpecLoc()
+                              : SourceLocation();
     }
 
     /// Retrieve the location of the 'restrict' qualifier.
     SourceLocation getRestrictQualifierLoc() const {
-      assert(MethodQualifiers);
-      return MethodQualifiers->getRestrictSpecLoc();
+      return MethodQualifiers ? MethodQualifiers->getRestrictSpecLoc()
+                              : SourceLocation();
     }
 
     /// Retrieve the location of the 'mutable' qualifier, if any.
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 0dbdf923df95a..22749dc4799bc 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -9203,15 +9203,15 @@ static NamedDecl *DiagnoseInvalidRedeclaration(
         << Idx << FDParam->getType()
         << NewFD->getParamDecl(Idx - 1)->getType();
     } else if (FDisConst != NewFDisConst) {
-      SemaRef.Diag(FD->getLocation(), diag::note_member_def_close_const_match)
-          << NewFDisConst << FD->getSourceRange().getEnd()
-          << (NewFDisConst
-                  ? FixItHint::CreateRemoval(ExtraArgs.D.getFunctionTypeInfo()
-                                                 .getConstQualifierLoc())
-                  : FixItHint::CreateInsertion(ExtraArgs.D.getFunctionTypeInfo()
-                                                   .getRParenLoc()
-                                                   .getLocWithOffset(1),
-                                               " const"));
+      auto DB = SemaRef.Diag(FD->getLocation(),
+                             diag::note_member_def_close_const_match)
+                << NewFDisConst << FD->getSourceRange().getEnd();
+      if (const auto &FTI = ExtraArgs.D.getFunctionTypeInfo(); !NewFDisConst)
+        DB << FixItHint::CreateInsertion(FTI.getRParenLoc().getLocWithOffset(1),
+                                         " const");
+      else if (SourceLocation ConstLoc = FTI.getConstQualifierLoc();
+               ConstLoc.isValid())
+        DB << FixItHint::CreateRemoval(ConstLoc);
     } else
       SemaRef.Diag(FD->getLocation(),
                    IsMember ? diag::note_member_def_close_match
diff --git a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p1.cpp
index a28a5f91c4775..788e93b56bb38 100644
--- a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p1.cpp
@@ -154,3 +154,14 @@ namespace {
   // FIXME: We should diagnose this prior to C++17.
   const int &r = A::n;
 }
+
+#if __cplusplus < 201402L
+namespace ImplicitConstexprDef {
+  struct A {
+    void f(); // expected-note {{member declaration does not match because it is not const qualified}}
+  };
+
+  constexpr void A::f() { } // expected-warning {{'constexpr' non-static member function will not be implicitly 'const' in C++14; add 'const' to avoid a change in behavior}}
+                            // expected-error@-1 {{out-of-line definition of 'f' does not match any declaration in 'ImplicitConstexprDef::A'}}
+}
+#endif

erichkeane

@AaronBallman is probably the best one to review DeclChunk stuff, perhaps @cor3ntin

erichkeane · 2024-05-16T20:56:28Z

clang/docs/ReleaseNotes.rst

@@ -724,6 +724,8 @@ Bug Fixes to C++ Support
  templates during partial ordering when deducing template arguments from a function declaration or when
  taking the address of a function template.
 - Fix a bug with checking constrained non-type template parameters for equivalence. Fixes (#GH77377).
+- Fix a C++11 crash when a non-const non-static member function is defined out-of-line with


Is there a bug # for this one?

Don't know, I just happened upon it when working on #92449... I'll check

I looked, but couldn't find any

Nevermind, I did :) #61004

erichkeane · 2024-05-16T20:59:46Z

clang/include/clang/Sema/DeclSpec.h

@@ -1527,20 +1527,20 @@ struct DeclaratorChunk {

    /// Retrieve the location of the 'const' qualifier.
    SourceLocation getConstQualifierLoc() const {
-      assert(MethodQualifiers);
-      return MethodQualifiers->getConstSpecLoc();
+      return MethodQualifiers ? MethodQualifiers->getConstSpecLoc()


Hmm... these asserts make sense in just about every situation except for the 'fixits'. I wonder if there is instead value in capturing that we did it with a 'fixit' instead? So the assert becomes MethodQualifiers || WasFixConstFixit.

DeclSpec.h is basically used only to gather information from the parser, and so from that perspective, I think the assert makes sense. If the caller is asking for the location of the const qualifier and one wasn't written in source, the caller is doing something out of the ordinary.

I think the fix-it code should be guarded by a call to hasMethodTypeQualifiers() instead.

@AaronBallman Updated

erichkeane · 2024-05-16T21:01:04Z

clang/lib/Sema/SemaDecl.cpp

+      auto DB = SemaRef.Diag(FD->getLocation(),
+                             diag::note_member_def_close_const_match)
+                << NewFDisConst << FD->getSourceRange().getEnd();
+      if (const auto &FTI = ExtraArgs.D.getFunctionTypeInfo(); !NewFDisConst)


Suggested change

if (const auto &FTI = ExtraArgs.D.getFunctionTypeInfo(); !NewFDisConst)

if (const auto &FTI = ExtraArgs.D.getFunctionTypeInfo(); NewFDisConst)

This is closer to what we mean, right?

Er, no. We want to suggest removing const when NewFDisConst is true, or suggest inserting it if false.

Woops, I forgot my other change :D I meant FDisConst since i saw the diagnostic was in terms of the 'old' value? But I could have been mistaken.

I think it would work either way... since this is guarded by if (FDisConst != NewFDisConst), !NewFDisConst implies FDisConst.

Yeah, I agree it is not really a meaningful change, besides being a bit of a 'say what you mean'. That is, use the variable that is more closely related to what we MEAN. I haven't reviewed this in long enough to know if I was correct here, so if you're sure that the variable you're using is the one that properly states the intent, feel free to ignore hte comment.

erichkeane · 2024-05-16T21:01:15Z

clang/lib/Sema/SemaDecl.cpp

+      if (const auto &FTI = ExtraArgs.D.getFunctionTypeInfo(); !NewFDisConst)
+        DB << FixItHint::CreateInsertion(FTI.getRParenLoc().getLocWithOffset(1),
+                                         " const");
+      else if (SourceLocation ConstLoc = FTI.getConstQualifierLoc();


Since this is the only use of it, I wonder if we could just better detect this case...

I opted for the "return SourceLocation() if no const qualifier is present" approach because it doesn't require the caller to check hasMethodTypeQualifiers() prior to calling getConstQualifierLoc(). Makes it less prone to being the cause of future crashes :)

I think that hides subtle issues; the caller should really know the difference between "const qualifier as written" and "semantically const".

Fair enough :)

AaronBallman

LGTM!

…declaration in C++11

…iers instead

…declaration in C++11 (llvm#92452) Clang crashes when diagnosing the following invalid redeclaration in C++11: ``` struct A { void f(); }; constexpr void A::f() { } // crash here ``` This happens because `DiagnoseInvalidRedeclaration` tries to create a fix-it to remove `const` from the out-of-line declaration of `f`, but there is no `SourceLocation` for the `const` qualifier (it's implicitly `const` due to `constexpr`) and an assert in `FunctionTypeInfo::getConstQualifierLoc` fails. This patch changes `DiagnoseInvalidRedeclaration` to only suggest the removal of the `const` qualifier when it was explicitly specified in the _cv-qualifier-seq_ of the declaration.

sdkrystian requested review from cor3ntin and erichkeane May 16, 2024 20:51

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

sdkrystian requested a review from Endilll May 16, 2024 20:52

erichkeane reviewed May 16, 2024

View reviewed changes

sdkrystian requested review from AaronBallman and erichkeane May 17, 2024 12:50

sdkrystian force-pushed the fix-crash-implicit-const branch from 569e2f1 to 46fd950 Compare May 17, 2024 13:00

AaronBallman approved these changes May 17, 2024

View reviewed changes

sdkrystian force-pushed the fix-crash-implicit-const branch from d7895e4 to e19f1d6 Compare May 17, 2024 21:01

sdkrystian added 3 commits May 20, 2024 10:55

[Clang][Sema] Fix crash when diagnosing near-match for 'constexpr' re…

6135600

…declaration in C++11

[FOLD] undo changes to FunctionTypeInfo and check hasMethodTypeQualif…

b5068cb

…iers instead

[FOLD] update release note

95d1cb2

sdkrystian force-pushed the fix-crash-implicit-const branch from e19f1d6 to 95d1cb2 Compare May 20, 2024 14:55

sdkrystian merged commit 3efaf9c into llvm:main May 20, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] Fix crash when diagnosing near-match for 'constexpr' redeclaration in C++11 #92452

[Clang][Sema] Fix crash when diagnosing near-match for 'constexpr' redeclaration in C++11 #92452

sdkrystian commented May 16, 2024

llvmbot commented May 16, 2024

erichkeane left a comment

erichkeane May 16, 2024

sdkrystian May 16, 2024

sdkrystian May 17, 2024

sdkrystian May 17, 2024

erichkeane May 16, 2024

AaronBallman May 17, 2024

sdkrystian May 17, 2024

erichkeane May 16, 2024

sdkrystian May 16, 2024

erichkeane May 17, 2024

sdkrystian May 17, 2024

erichkeane May 20, 2024

erichkeane May 16, 2024

sdkrystian May 16, 2024

AaronBallman May 17, 2024

sdkrystian May 17, 2024

AaronBallman left a comment

	if (const auto &FTI = ExtraArgs.D.getFunctionTypeInfo(); !NewFDisConst)
	if (const auto &FTI = ExtraArgs.D.getFunctionTypeInfo(); NewFDisConst)

[Clang][Sema] Fix crash when diagnosing near-match for 'constexpr' redeclaration in C++11 #92452

[Clang][Sema] Fix crash when diagnosing near-match for 'constexpr' redeclaration in C++11 #92452

Conversation

sdkrystian commented May 16, 2024

llvmbot commented May 16, 2024

erichkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment