[Serialization] Handle uninitialized type constraints #110496

falbrechtskirchinger · 2024-09-30T11:48:06Z

The ASTWriter currently assumes template type constraints to be initialized ((bool)getTypeConstraint() == hasTypeConstraint()). The attached test case presents a scenario where that is not the case.

This patch removes the assumption and adds another boolean to the serialization, to explicitly encode whether the type constraint has been initialized.

Fixes #99036.
Fixes #109354.

github-actions · 2024-09-30T11:48:24Z

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.

llvmbot · 2024-09-30T11:48:57Z

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clangd

Author: Florian Albrechtskirchinger (falbrechtskirchinger)

Changes

Note: This patch is based on the assumption, that an assertion causing a crash in clangd, is incorrect. Alternatively, the true error may lie elsewhere, possibly in ´Sema::BuildTypeConstraint/Sema::AttachTypeConstraint`. Someone with more experience is needed to make that call.
Commit message follows.

<hr />

The ASTWriter currently assumes template type constraints to be initialized ((bool)getTypeConstraint() == hasTypeConstraint()). The attached test case presents a scenario where that is not the case.

This patch removes the assumption and adds another boolean to the serialization, to explicitly encode whether the type constraint has been initialized.

Fixes #99036.
Fixes #109354.

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

3 Files Affected:

(added) clang-tools-extra/clangd/test/GH99036.test (+29)
(modified) clang/lib/Serialization/ASTReaderDecl.cpp (+1-1)
(modified) clang/lib/Serialization/ASTWriterDecl.cpp (+3-2)

diff --git a/clang-tools-extra/clangd/test/GH99036.test b/clang-tools-extra/clangd/test/GH99036.test
new file mode 100644
index 00000000000000..64ebe4c75a999d
--- /dev/null
+++ b/clang-tools-extra/clangd/test/GH99036.test
@@ -0,0 +1,29 @@
+# RUN: rm -rf %t
+# RUN: mkdir -p %t
+# RUN: split-file %s %t
+#
+# RUN: not clangd --check=%t/main.cpp 2>&1 | FileCheck -strict-whitespace %s
+#
+# CHECK:      Loaded compilation database
+# CHECK-NEXT: Compile command inferred from main.cpp is:
+# CHECK:      Indexing headers...
+# CHECK-NEXT: [unknown_typename] Line 1: in included file: unknown type name '_Up'
+# CHECK:      All checks completed, 1 errors
+
+#--- header.h
+template<_Up>
+concept __decayed_same_as;
+template<__decayed_same_as>
+partial_ordering operator0
+
+#--- main.cpp
+#include "header.h"
+
+#--- compile_commands.json
+[
+  {
+    "directory": "/",
+    "command": "c++ -std=c++20",
+    "file": "main.cpp"
+  }
+]
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 7cead2728ca938..90783963934bb0 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -2674,7 +2674,7 @@ void ASTDeclReader::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
 
   D->setDeclaredWithTypename(Record.readInt());
 
-  if (D->hasTypeConstraint()) {
+  if (Record.readBool() && D->hasTypeConstraint()) {
     ConceptReference *CR = nullptr;
     if (Record.readBool())
       CR = Record.readConceptReference();
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp b/clang/lib/Serialization/ASTWriterDecl.cpp
index b71684569609ac..1c9898c042cd4b 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1899,7 +1899,7 @@ void ASTDeclWriter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   Record.push_back(D->wasDeclaredWithTypename());
 
   const TypeConstraint *TC = D->getTypeConstraint();
-  assert((bool)TC == D->hasTypeConstraint());
+  Record.push_back(TC != nullptr); // reflects TypeConstraintInitialized
   if (TC) {
     auto *CR = TC->getConceptReference();
     Record.push_back(CR != nullptr);
@@ -1917,7 +1917,7 @@ void ASTDeclWriter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
   if (OwnsDefaultArg)
     Record.AddTemplateArgumentLoc(D->getDefaultArgument());
 
-  if (!TC && !OwnsDefaultArg &&
+  if (!D->hasTypeConstraint() && !OwnsDefaultArg &&
       D->getDeclContext() == D->getLexicalDeclContext() &&
       !D->isInvalidDecl() && !D->hasAttrs() &&
       !D->isTopLevelDeclInObjCContainer() && !D->isImplicit() &&
@@ -2580,6 +2580,7 @@ void ASTWriter::WriteDeclAbbrevs() {
   // TemplateTypeParmDecl
   Abv->Add(
       BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // wasDeclaredWithTypename
+  Abv->Add(BitCodeAbbrevOp(0));                    // TypeConstraintInitialized
   Abv->Add(BitCodeAbbrevOp(0));                    // OwnsDefaultArg
   DeclTemplateTypeParmAbbrev = Stream.EmitAbbrev(std::move(Abv));

zyn0217

Thanks again for analyzing & working on this issue.

Some suggestions regarding the test:

As this is more of a clang issue (the offending code belongs to the serialization part of clang), so it is more appropriate to write a clang test instead of a clangd test.
We don't usually write lit tests but rather unit tests (specifically, tests under the clangd/unittests directory) for clangd.

The other part LG in general, but I'd like to hear input from folks who are more familiar with the serialization.

clang/lib/Serialization/ASTWriterDecl.cpp

falbrechtskirchinger · 2024-09-30T17:19:05Z

Thanks again for analyzing & working on this issue.

You’re very welcome! Let's hope I'm on the right track.

Some suggestions regarding the test:

1. As this is more of a clang issue (the offending code belongs to the serialization part of clang), so it is more appropriate to write a clang test instead of a clangd test.

2. We don't usually write lit tests but rather unit tests (specifically, tests under the `clangd/unittests` directory) for clangd.

The most recent push contains a clangd unit test. I'd argue that replicating the test logic present in clangd (TestTU, ParsedAST) in clang unit tests is unnecessary. Also, the error is specific to clangd. No one has proposed a method of triggering the assertion using clang directly.

I've verified that the test triggers the assertion without the changes introduced in this PR.

[ RUN      ] ClangdAST.HandleUninitializedTypeConstraints
ClangdTests: /data6/git/llvm-project/clang/lib/Serialization/ASTWriterDecl.cpp:1902: void clang::ASTDeclWriter::VisitTemplateTypeParmDecl(clang::TemplateTypeParmDecl*): Assertion `(bool)TC == D->hasTypeConstraint()' failed.

If you insist on a clang unit test, I'll need help. :)

zyn0217 · 2024-10-01T02:55:35Z

@falbrechtskirchinger

We need to resort to -fallow-pch-with-compiler-errors :)

Specifically, add the following file to clang/test/PCH (or somehow merge it into a pre-existing test, which I'd prefer)

Name it with a reasonable name e.g. cxx2a-invalid-constraint-serialization.cpp

// RUN: %clang_cc1 -std=c++2a -emit-pch -fallow-pch-with-compiler-errors %s -o %t
// RUN: %clang_cc1 -std=c++2a -include-pch %t -verify %s

namespace GH99036 {

template <typename T>
concept C;

template <C U> void f();

} // namespace GH99036

Running cmake ... --target=check-clang, or with llvm-lit ./llvm-lit -sv /path/to/clang/test/PCH/cxx2a-invalid-constraint-serialization.cpp, and you can see a crash without your patch.

falbrechtskirchinger · 2024-10-01T06:25:56Z

We need to resort to -fallow-pch-with-compiler-errors :)

Ah, thanks! I would not have figured that out on my own. :)

Specifically, add the following file to clang/test/PCH (or somehow merge it into a pre-existing test, which I'd prefer)

I looked through that directory and found only one test with -fallow-pch-with-compiler-errors and -std=c++20: race-condition.cpp – not a great fit.

cxx2a-constraints-crash.cpp sounds promising and doesn't need too much modification. I'm including this in my next push, but I have an annotated standalone file based on your suggestion ready to go if needed.

clang/test/PCH/cxx2a-constraints-crash.cpp

zyn0217

Thanks! LGTM now

I'll help you merge it a few days later in case anyone else has further comments.

The ASTWriter currently assumes template type constraints to be initialized ((bool)getTypeConstraint() == hasTypeConstraint()). The attached test case presents a scenario where that is not the case. This patch removes the assumption and adds another boolean to the serialization, to explicitly encode whether the type constraint has been initialized. Fixes llvm#99036 Fixes llvm#109354

vient · 2024-10-07T12:38:30Z

Will this be backported to 19.x afterwards?

zyn0217 · 2024-10-07T13:12:32Z

@vient I think we can, yes

falbrechtskirchinger · 2024-10-07T13:14:23Z

@zyn0217 #111179 has fixed this crash as well. Should I strip this PR down to the unit test, or does it still make sense to you, to keep the changes?

Will this be backported to 19.x afterwards?

I was also going to ask, though, it's now a question about #111179.

cor3ntin · 2024-10-07T13:16:48Z

Is that change still necessary at all after #111179 ?

zyn0217 · 2024-10-07T13:17:08Z

#111179 aims for a different case (and that is a follow-up for @cor3ntin's recursive concept patch, which is not yet merged into 19, I think), while this one is related to a serialization bug that has persisted for some time.

So yes, this one is still necessary - it should occur with an assertion-enabled build when compiling a header containing an invalid concept decl.

falbrechtskirchinger · 2024-10-07T13:19:02Z

So yes, this one is still necessary - it should occur with an assertion-enabled build when compiling a header containing an invalid concept decl.

Not in my testing.

@zyn0217 If you'd like to check:
clang/test/PCH/cxx2a-constraints-crash.cpp

namespace GH99036 {

template <typename T>
concept C; // expected-error {{expected '='}}

template <C U> void f();

} // namespace GH99036

clang/lib/Serialization/ASTReaderDecl.cpp

cor3ntin · 2024-10-07T13:20:34Z

clang/lib/Serialization/ASTWriterDecl.cpp

@@ -1899,7 +1899,7 @@ void ASTDeclWriter::VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
  Record.push_back(D->wasDeclaredWithTypename());

  const TypeConstraint *TC = D->getTypeConstraint();
-  assert((bool)TC == D->hasTypeConstraint());
+  Record.push_back(/*TypeConstraintInitialized=*/TC != nullptr);
  if (TC) {


What about checking for D->hasTypeConstraint() instead of pushing am additional bool?

Because D->hasTypeConstraint() does not imply that D->getTypeConstraint() is non-null, as the code assumed (hence, the assertion).

zyn0217 · 2024-10-07T13:30:59Z

@falbrechtskirchinger Ooops, yes, it's true that the bug is gone w/ #111179.

But I'd say if we're going to cherry-pick #111179, we have to backport its precedent #103867 as well, so I think we can probably go ahead with this one and backport it to 19, and then we can revert this one on the trunk (tests should definitely be preserved) - this is indeed a bit awkward, but it depends whether it's worth backporting #103867 @cor3ntin

(Again, I apologize for not looking into it so carefully)

falbrechtskirchinger · 2024-10-07T13:36:30Z

@falbrechtskirchinger Ooops, yes, it's true that the bug is gone w/ #111179.

But I'd say if we're going to cherry-pick #111179, we have to backport its precedent #103867 as well, so I think we can probably go ahead with this one and backport it to 19, and then we can revert this one on the trunk - this is indeed a bit awkward, but it depends whether it's worth backporting #103867 @cor3ntin

I'd like this crash to be fixed in 19.x.

(Again, I apologize for not looking into it so carefully)

No worries.

To summarize: Land this PR and backport it. Revert on trunk. Submit new PR for the regression test? (We want to keep that, right?)

zyn0217 · 2024-10-07T13:39:03Z

To summarize: Land this PR and backport it. Revert on trunk. Submit new PR for the regression test? (We want to keep that, right?)

That's just my idea, and I'd like Corentin to say something about it :)

ChuanqiXu9

I'll leave this to @cor3ntin

vient · 2024-10-11T12:06:03Z

#111179 is merged now

vient · 2024-10-18T13:08:41Z

Bump, it would be nice to have this or that fix backported to 19.x

cor3ntin · 2024-10-21T14:29:21Z

I think the best past forward would be to close this PR, and to open a new one targeting the 10.x branch, which a clear explanation that it's not needed in trunk. would that work for you @falbrechtskirchinger ?

falbrechtskirchinger · 2024-10-21T15:16:35Z

I think the best past forward would be to close this PR, and to open a new one targeting the 10.x branch, which a clear explanation that it's not needed in trunk. would that work for you @falbrechtskirchinger ?

Will do.

falbrechtskirchinger · 2024-10-21T16:19:36Z

Bump, it would be nice to have this or that fix backported to 19.x

@vient Backport is happening. #113182

llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd clang:modules C++20 modules and Clang Header Modules labels Sep 30, 2024

falbrechtskirchinger force-pushed the handle-uninit-tcs branch from 296cc1b to 20042cc Compare September 30, 2024 13:14

zyn0217 reviewed Sep 30, 2024

View reviewed changes

clang/lib/Serialization/ASTWriterDecl.cpp Outdated Show resolved Hide resolved

zyn0217 requested review from erichkeane and ChuanqiXu9 September 30, 2024 13:49

falbrechtskirchinger force-pushed the handle-uninit-tcs branch from 20042cc to 8913503 Compare September 30, 2024 17:14

falbrechtskirchinger force-pushed the handle-uninit-tcs branch from 8913503 to c090601 Compare October 1, 2024 06:33

falbrechtskirchinger commented Oct 1, 2024

View reviewed changes

clang/test/PCH/cxx2a-constraints-crash.cpp Outdated Show resolved Hide resolved

falbrechtskirchinger changed the title ~~[clangd] [AST] Handle uninitialized type constraints~~ [Serialization] Handle uninitialized type constraints Oct 1, 2024

zyn0217 approved these changes Oct 1, 2024

View reviewed changes

falbrechtskirchinger force-pushed the handle-uninit-tcs branch from c090601 to 220d76a Compare October 2, 2024 04:57

zyn0217 mentioned this pull request Oct 4, 2024

clang 20 crashes on invalid input #109780

Closed

cor3ntin reviewed Oct 7, 2024

View reviewed changes

ChuanqiXu9 reviewed Oct 8, 2024

View reviewed changes

falbrechtskirchinger mentioned this pull request Oct 21, 2024

[Concepts] Add regression test for #99036 #113137

Merged

falbrechtskirchinger closed this Oct 21, 2024

falbrechtskirchinger mentioned this pull request Oct 21, 2024

[Serialization] Handle uninitialized type constraints #113182

Merged

falbrechtskirchinger deleted the handle-uninit-tcs branch October 21, 2024 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serialization] Handle uninitialized type constraints #110496

[Serialization] Handle uninitialized type constraints #110496

falbrechtskirchinger commented Sep 30, 2024 •

edited

Loading

github-actions bot commented Sep 30, 2024

llvmbot commented Sep 30, 2024 •

edited

Loading

zyn0217 left a comment

falbrechtskirchinger commented Sep 30, 2024

zyn0217 commented Oct 1, 2024 •

edited

Loading

falbrechtskirchinger commented Oct 1, 2024

zyn0217 left a comment

vient commented Oct 7, 2024

zyn0217 commented Oct 7, 2024

falbrechtskirchinger commented Oct 7, 2024

cor3ntin commented Oct 7, 2024

zyn0217 commented Oct 7, 2024 •

edited

Loading

falbrechtskirchinger commented Oct 7, 2024 •

edited

Loading

cor3ntin Oct 7, 2024

falbrechtskirchinger Oct 7, 2024

zyn0217 commented Oct 7, 2024 •

edited

Loading

falbrechtskirchinger commented Oct 7, 2024

zyn0217 commented Oct 7, 2024

ChuanqiXu9 left a comment

vient commented Oct 11, 2024

vient commented Oct 18, 2024

cor3ntin commented Oct 21, 2024

falbrechtskirchinger commented Oct 21, 2024

falbrechtskirchinger commented Oct 21, 2024

[Serialization] Handle uninitialized type constraints #110496

[Serialization] Handle uninitialized type constraints #110496

Conversation

falbrechtskirchinger commented Sep 30, 2024 • edited Loading

github-actions bot commented Sep 30, 2024

llvmbot commented Sep 30, 2024 • edited Loading

zyn0217 left a comment

Choose a reason for hiding this comment

falbrechtskirchinger commented Sep 30, 2024

zyn0217 commented Oct 1, 2024 • edited Loading

falbrechtskirchinger commented Oct 1, 2024

zyn0217 left a comment

Choose a reason for hiding this comment

vient commented Oct 7, 2024

zyn0217 commented Oct 7, 2024

falbrechtskirchinger commented Oct 7, 2024

cor3ntin commented Oct 7, 2024

zyn0217 commented Oct 7, 2024 • edited Loading

falbrechtskirchinger commented Oct 7, 2024 • edited Loading

cor3ntin Oct 7, 2024

Choose a reason for hiding this comment

falbrechtskirchinger Oct 7, 2024

Choose a reason for hiding this comment

zyn0217 commented Oct 7, 2024 • edited Loading

falbrechtskirchinger commented Oct 7, 2024

zyn0217 commented Oct 7, 2024

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

vient commented Oct 11, 2024

vient commented Oct 18, 2024

cor3ntin commented Oct 21, 2024

falbrechtskirchinger commented Oct 21, 2024

falbrechtskirchinger commented Oct 21, 2024

falbrechtskirchinger commented Sep 30, 2024 •

edited

Loading

llvmbot commented Sep 30, 2024 •

edited

Loading

zyn0217 commented Oct 1, 2024 •

edited

Loading

zyn0217 commented Oct 7, 2024 •

edited

Loading

falbrechtskirchinger commented Oct 7, 2024 •

edited

Loading

zyn0217 commented Oct 7, 2024 •

edited

Loading