[clang] Serialization: support hashing null template arguments #141890
Conversation
When performing overload resolution during code completion, clang will allow incomplete substitutions in more places than would be allowed for valid code, because for completion to work well, it needs clang to keep going so it can explore the space of possibilities. Notably, we accept instantiating declarations will null template arguments, and this works fine, except that when lazily loading serialzied templated declarations, the template argument hasher assumes null arguments can't be used. This patch makes the hasher happily accept that. Fixes #139019
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Matheus Izvekov (mizvekov) ChangesWhen performing overload resolution during code completion, clang will allow incomplete substitutions in more places than would be allowed for valid code, because for completion to work well, it needs clang to keep going so it can explore the space of possibilities. Notably, we accept instantiating declarations will null template arguments, and this works fine, except that when lazily loading serialzied templated declarations, the template argument hasher assumes null arguments can't be used. This patch makes the hasher happily accept that. Fixes #139019 Full diff: https://github.com/llvm/llvm-project/pull/141890.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 540552bc47e8d..32266fce4d3cb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -821,6 +821,7 @@ Miscellaneous Clang Crashes Fixed
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- Fixed crash when ``-print-stats`` is enabled in compiling IR files. (#GH131608)
+- Fix code completion crash involving PCH serialzied templates. (#GH139019)
OpenACC Specific Changes
------------------------
diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp b/clang/lib/Serialization/TemplateArgumentHasher.cpp
index 3c7177b83ba52..aa61496d4aa0c 100644
--- a/clang/lib/Serialization/TemplateArgumentHasher.cpp
+++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp
@@ -65,7 +65,9 @@ void TemplateArgumentHasher::AddTemplateArgument(TemplateArgument TA) {
switch (Kind) {
case TemplateArgument::Null:
- llvm_unreachable("Expected valid TemplateArgument");
+ // These can occur in incomplete substitutions performed with code
+ // completion (see PartialOverloading).
+ break;
case TemplateArgument::Type:
AddQualType(TA.getAsType());
break;
diff --git a/clang/test/CodeCompletion/GH139019.cpp b/clang/test/CodeCompletion/GH139019.cpp
new file mode 100644
index 0000000000000..fed35b38362a1
--- /dev/null
+++ b/clang/test/CodeCompletion/GH139019.cpp
@@ -0,0 +1,26 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/test.hpp -emit-pch -o %t/1.pch
+// RUN: %clang_cc1 -std=c++20 %t/test.cpp -include-pch %t/1.pch -code-completion-at=%t/test.cpp:7:17
+
+//--- test.hpp
+#pragma once
+class provider_t
+{
+ public:
+ template<class T>
+ void emit(T *data)
+ {}
+};
+
+//--- test.cpp
+#include "test.hpp"
+
+void test()
+{
+ provider_t *focus;
+ void *data;
+ focus->emit(&data);
+}
|
|
@llvm/pr-subscribers-clang-modules Author: Matheus Izvekov (mizvekov) ChangesWhen performing overload resolution during code completion, clang will allow incomplete substitutions in more places than would be allowed for valid code, because for completion to work well, it needs clang to keep going so it can explore the space of possibilities. Notably, we accept instantiating declarations will null template arguments, and this works fine, except that when lazily loading serialzied templated declarations, the template argument hasher assumes null arguments can't be used. This patch makes the hasher happily accept that. Fixes #139019 Full diff: https://github.com/llvm/llvm-project/pull/141890.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 540552bc47e8d..32266fce4d3cb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -821,6 +821,7 @@ Miscellaneous Clang Crashes Fixed
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- Fixed crash when ``-print-stats`` is enabled in compiling IR files. (#GH131608)
+- Fix code completion crash involving PCH serialzied templates. (#GH139019)
OpenACC Specific Changes
------------------------
diff --git a/clang/lib/Serialization/TemplateArgumentHasher.cpp b/clang/lib/Serialization/TemplateArgumentHasher.cpp
index 3c7177b83ba52..aa61496d4aa0c 100644
--- a/clang/lib/Serialization/TemplateArgumentHasher.cpp
+++ b/clang/lib/Serialization/TemplateArgumentHasher.cpp
@@ -65,7 +65,9 @@ void TemplateArgumentHasher::AddTemplateArgument(TemplateArgument TA) {
switch (Kind) {
case TemplateArgument::Null:
- llvm_unreachable("Expected valid TemplateArgument");
+ // These can occur in incomplete substitutions performed with code
+ // completion (see PartialOverloading).
+ break;
case TemplateArgument::Type:
AddQualType(TA.getAsType());
break;
diff --git a/clang/test/CodeCompletion/GH139019.cpp b/clang/test/CodeCompletion/GH139019.cpp
new file mode 100644
index 0000000000000..fed35b38362a1
--- /dev/null
+++ b/clang/test/CodeCompletion/GH139019.cpp
@@ -0,0 +1,26 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/test.hpp -emit-pch -o %t/1.pch
+// RUN: %clang_cc1 -std=c++20 %t/test.cpp -include-pch %t/1.pch -code-completion-at=%t/test.cpp:7:17
+
+//--- test.hpp
+#pragma once
+class provider_t
+{
+ public:
+ template<class T>
+ void emit(T *data)
+ {}
+};
+
+//--- test.cpp
+#include "test.hpp"
+
+void test()
+{
+ provider_t *focus;
+ void *data;
+ focus->emit(&data);
+}
|
github-project-automation
bot
moved this from Needs Triage
to Needs Merge
in LLVM Release Status
github-project-automation
bot
moved this from Needs Merge
to Done
in LLVM Release Status
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/23407 Here is the relevant piece of the build log for the reference |
|
/cherry-pick 6131407 |
|
Failed to cherry-pick: 6131407 https://github.com/llvm/llvm-project/actions/runs/15317560723 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
| @@ -65,7 +65,9 @@ void TemplateArgumentHasher::AddTemplateArgument(TemplateArgument TA) { | |||
|
|
|||
| switch (Kind) { | |||
| case TemplateArgument::Null: | |||
| llvm_unreachable("Expected valid TemplateArgument"); | |||
| // These can occur in incomplete substitutions performed with code | |||
| // completion (see PartialOverloading). | |||
There was a problem hiding this comment.
see PartialOverloading where? Is there a specific method or API doc we should look at?
There was a problem hiding this comment.
It's the term of art within clang, and the name of the flag as used in source code.
A search on doxygen for this term should lead to:
/// AddOverloadCandidate - Adds the given function to the set of
/// candidate functions, using the given function call arguments. If
/// @p SuppressUserConversions, then don't allow user-defined
/// conversions via constructors or conversion operators.
///
/// \param PartialOverloading true if we are performing "partial" overloading
/// based on an incomplete set of function arguments. This feature is used by
/// code completion.This term can also easily be greped for, and there are no irrelevant results.
But this is mostly undocumented.
There was a problem hiding this comment.
So I am using Visual Studio code and did a naive search for PartialOverloading and it was definitely not obvious where I specifically should look for this term. I did spend some time trying to dig around.
I think my point is that the comment is not super helpful unless you already have some understanding. So it requires a bit more elaboration so that random folks who come upon can get it.
There was a problem hiding this comment.
I think this name is the best stable reference to this functionality.
And I don't think mentioning file names is a good idea, and there is no documentation to point to.
Is there any specific thing this comment should say so you think this would be easier to find?
|
For the record, the manual backport PR is at: #141957 |
When performing overload resolution during code completion, clang will allow incomplete substitutions in more places than would be allowed for valid code, because for completion to work well, it needs clang to keep going so it can explore the space of possibilities.
Notably, we accept instantiating declarations with null template arguments, and this works fine, except that when lazily loading serialized templated declarations, the template argument hasher assumes null arguments can't be used.
This patch makes the hasher happily accept that.
Fixes #139019