[Clang] Fix crash on template-specialization #142338

mark-de-wever-sonarsource · 2025-06-02T07:01:30Z

This applies the name restoration as suggested by Richard Smith.

This applies the name restoration as suggested by Richard Smith. Fixes: llvm#54279

github-actions · 2025-06-02T07:01:51Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

mark-de-wever-sonarsource · 2025-06-02T07:02:15Z

clang/test/CodeGenCXX/constructor-init.cpp

@@ -160,7 +160,7 @@ template<typename T> struct X;

 // Make sure that the instantiated constructor initializes start and
 // end properly.
-// CHECK-LABEL: define linkonce_odr void @_ZN1XIiEC2ERKS0_(ptr {{[^,]*}} %this, ptr noundef nonnull align {{[0-9]+}} dereferenceable({{[0-9]+}}) %other) unnamed_addr
+// CHECK-LABEL: define linkonce_odr void @_ZN1XIiEC2ERKS0_(ptr {{[^,]*}} %this, ptr noundef nonnull align {{[0-9]+}} dereferenceable({{[0-9]+}}) %0) unnamed_addr


The restoration of the name changes the param name in the AST and causes this test to fail.
It seems possible to avoid it, but I'm not sure the current solution is bad on the case for the default argument.

There seem to be no AST tests that validate this requirement.

What name does it change it to? Is this expected?

It changed the emitted definition from X<int>::X(const X &other) to X<int>::X(const X&) IIUC. I'm not sure if this is a benign change or a regression - do we need the parameter name in the middle-end?

It feels weird but not really sure. The change seemed to be about restoring names and so I find the result puzzling.

It changes to the name used in the declaration. In this case the declaration in the class is X(const X &); and the argument has no name. In this case it uses %0. Would the declaration be X(const X & x); the name would change to %x.

I noticed only one test fails due to the change and it does not look that the name of the argument is specifically part of the test. There seem to be no other tests that are affected. So I'm not sure whether this specific change in behaviour is wanted or important. I'd be happy to look into not changing it in this case.

llvmbot · 2025-06-02T07:02:29Z

@llvm/pr-subscribers-clang

Author: Mark de Wever (mark-de-wever-sonarsource)

Changes

This applies the name restoration as suggested by Richard Smith.

Fixes: #54279

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

3 Files Affected:

(modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+5)
(modified) clang/test/CodeGenCXX/constructor-init.cpp (+1-1)
(modified) clang/test/SemaTemplate/default-arguments.cpp (+9)

diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 174c8fc59e4fa..9853466e01496 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -34,6 +34,7 @@
 #include "clang/Sema/SemaSwift.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateInstCallback.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/TimeProfiler.h"
 #include <optional>
 
@@ -5120,6 +5121,10 @@ bool Sema::addInstantiatedParametersToScope(
       // Simple case: not a parameter pack.
       assert(FParamIdx < Function->getNumParams());
       ParmVarDecl *FunctionParam = Function->getParamDecl(FParamIdx);
+      DeclarationName name = FunctionParam->getDeclName();
+      auto _ = llvm::make_scope_exit([&]() {
+        FunctionParam->setDeclName(name);
+      });
       FunctionParam->setDeclName(PatternParam->getDeclName());
       // If the parameter's type is not dependent, update it to match the type
       // in the pattern. They can differ in top-level cv-qualifiers, and we want
diff --git a/clang/test/CodeGenCXX/constructor-init.cpp b/clang/test/CodeGenCXX/constructor-init.cpp
index f191599f360e7..b5f4cc0b3d6c8 100644
--- a/clang/test/CodeGenCXX/constructor-init.cpp
+++ b/clang/test/CodeGenCXX/constructor-init.cpp
@@ -160,7 +160,7 @@ template<typename T> struct X;
 
 // Make sure that the instantiated constructor initializes start and
 // end properly.
-// CHECK-LABEL: define linkonce_odr void @_ZN1XIiEC2ERKS0_(ptr {{[^,]*}} %this, ptr noundef nonnull align {{[0-9]+}} dereferenceable({{[0-9]+}}) %other) unnamed_addr
+// CHECK-LABEL: define linkonce_odr void @_ZN1XIiEC2ERKS0_(ptr {{[^,]*}} %this, ptr noundef nonnull align {{[0-9]+}} dereferenceable({{[0-9]+}}) %0) unnamed_addr
 // CHECK: {{store.*null}}
 // CHECK: {{store.*null}}
 // CHECK: ret
diff --git a/clang/test/SemaTemplate/default-arguments.cpp b/clang/test/SemaTemplate/default-arguments.cpp
index 5ea34c0254ec1..a366c3d8ab722 100644
--- a/clang/test/SemaTemplate/default-arguments.cpp
+++ b/clang/test/SemaTemplate/default-arguments.cpp
@@ -283,3 +283,12 @@ static_assert(S<short *>().SizeOfT<char>() == sizeof(short *), "");
 } // namespace GH68490
 
 #endif
+
+namespace PR54279 {
+// Using a different name for the argument when there is a default argument
+// caused a crash.
+template <typename T> void f(const T &a, int c = 0);
+template <> void f(const int &unused, int) {
+  f(42);
+}
+}

tbaederr · 2025-06-02T07:24:18Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -5120,6 +5121,10 @@ bool Sema::addInstantiatedParametersToScope(
      // Simple case: not a parameter pack.
      assert(FParamIdx < Function->getNumParams());
      ParmVarDecl *FunctionParam = Function->getParamDecl(FParamIdx);
+      DeclarationName name = FunctionParam->getDeclName();


Suggested change

DeclarationName name = FunctionParam->getDeclName();

DeclarationName Name = FunctionParam->getDeclName();

cor3ntin · 2025-06-02T07:54:56Z

This change needs a release note.
Please add an entry to clang/docs/ReleaseNotes.rst in the section the most adapted to the change, and referencing any Github issue this change fixes. Thanks!

erichkeane · 2025-06-02T13:57:33Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -5120,6 +5121,10 @@ bool Sema::addInstantiatedParametersToScope(
      // Simple case: not a parameter pack.
      assert(FParamIdx < Function->getNumParams());
      ParmVarDecl *FunctionParam = Function->getParamDecl(FParamIdx);
+      DeclarationName name = FunctionParam->getDeclName();
+      auto _ = llvm::make_scope_exit([&]() {
+        FunctionParam->setDeclName(name);


Something about this being done at scope-exist makes me think this is a smell. I don't think I understand what is going on here enough with the various names enough to know whether this is OK. Could @zygoloid take a quick look and see if this is crazy or not?

This approach is based on what Richard suggested in #54279 (comment) " for example, we need to set the name back when we're done with it.".

The crash is caused due to another scope-exit. In that scope a FETokenInfo "attached" to the name of the function argument. This function is unaware of that scope so the "rename" is unknown to that scope and at that scope's scope-exit the information "attached" to the current name (not the original name) is removed. This is where an assertion triggers.

So this change basically restores the original name so the other scope-exit will "detach" the FETokenInfo using the original name and the crash is resolved.

I notice we have a FunctionParam->setDeclName(PatternParam->getDeclName()); not far below this point. Do we also need to apply a fix there?

I notice we have a FunctionParam->setDeclName(PatternParam->getDeclName()); not far below this point. Do we also need to apply a fix there?

No? Unless we allowed default arguments on parameter packs

zyn0217 · 2025-06-03T16:34:10Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

+      DeclarationName Name = FunctionParam->getDeclName();
+      auto _ = llvm::make_scope_exit([&]() {
+        FunctionParam->setDeclName(Name);
+      });


We can always init-capture it

Suggested change

DeclarationName Name = FunctionParam->getDeclName();

auto _ = llvm::make_scope_exit([&]() {

FunctionParam->setDeclName(Name);

});

auto _ = llvm::make_scope_exit([&, Name(FunctionParam->getDeclName())] {

FunctionParam->setDeclName(Name);

});

zyn0217 · 2025-06-03T16:39:05Z

clang/test/CodeGenCXX/constructor-init.cpp

@@ -160,7 +160,7 @@ template<typename T> struct X;

 // Make sure that the instantiated constructor initializes start and
 // end properly.
-// CHECK-LABEL: define linkonce_odr void @_ZN1XIiEC2ERKS0_(ptr {{[^,]*}} %this, ptr noundef nonnull align {{[0-9]+}} dereferenceable({{[0-9]+}}) %other) unnamed_addr
+// CHECK-LABEL: define linkonce_odr void @_ZN1XIiEC2ERKS0_(ptr {{[^,]*}} %this, ptr noundef nonnull align {{[0-9]+}} dereferenceable({{[0-9]+}}) %0) unnamed_addr


It changed the emitted definition from X<int>::X(const X &other) to X<int>::X(const X&) IIUC. I'm not sure if this is a benign change or a regression - do we need the parameter name in the middle-end?

Fix crash on template-specialization

930e073

This applies the name restoration as suggested by Richard Smith. Fixes: llvm#54279

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 2, 2025

mark-de-wever-sonarsource commented Jun 2, 2025

View reviewed changes

mark-de-wever-sonarsource changed the title ~~Fix crash on template-specialization~~ [Clang] Fix crash on template-specialization Jun 2, 2025

tbaederr reviewed Jun 2, 2025

View reviewed changes

zyn0217 requested review from zyn0217 and cor3ntin June 2, 2025 07:42

cor3ntin requested review from zygoloid and erichkeane June 2, 2025 11:58

erichkeane reviewed Jun 2, 2025

View reviewed changes

mark-de-wever-sonarsource mentioned this pull request Jun 3, 2025

Clang-19 crash: void* clang::DeclarationName::getFETokenInfo() const: Assertion `getPtr() && "getFETokenInfo on an empty DeclarationName!"' failed. #95420

Open

Address review comments

25d0d5d

zyn0217 reviewed Jun 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Fix crash on template-specialization #142338

[Clang] Fix crash on template-specialization #142338

Uh oh!

mark-de-wever-sonarsource commented Jun 2, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jun 2, 2025

Uh oh!

mark-de-wever-sonarsource Jun 2, 2025

Uh oh!

shafik Jun 3, 2025

Uh oh!

zyn0217 Jun 3, 2025

Uh oh!

shafik Jun 4, 2025

Uh oh!

mark-de-wever-sonarsource Jun 4, 2025

Uh oh!

llvmbot commented Jun 2, 2025

Uh oh!

tbaederr Jun 2, 2025

Uh oh!

cor3ntin commented Jun 2, 2025

Uh oh!

erichkeane Jun 2, 2025

Uh oh!

mark-de-wever-sonarsource Jun 2, 2025

Uh oh!

shafik Jun 3, 2025

Uh oh!

zyn0217 Jun 3, 2025

Uh oh!

zyn0217 Jun 3, 2025

Uh oh!

zyn0217 Jun 3, 2025

Uh oh!

Uh oh!

	DeclarationName name = FunctionParam->getDeclName();
	DeclarationName Name = FunctionParam->getDeclName();

[Clang] Fix crash on template-specialization #142338

Are you sure you want to change the base?

[Clang] Fix crash on template-specialization #142338

Uh oh!

Conversation

mark-de-wever-sonarsource commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cor3ntin commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mark-de-wever-sonarsource commented Jun 2, 2025 •

edited

Loading