[clang] Fix array types comparison in getCommonSugaredType #131649

spavloff · 2025-03-17T17:39:09Z

Const-qualification of an array caused by constexpr specifier can produce QualType, where the const qualifier is set both as fast qualifier and as a qualifier of the array element type. It can result in a compiler crash, because such QualType does not compare equal to the same type but without extra qualification.

To avoid the crash, the redundant qualifiers are removed while searching for common sugar.

It fixes #97005 (Clang crashed in ASTContext::getCommonSugaredType).

Const-qualification of an array caused by constexpr specifier can produce QualType, where the const qualifier is set both as fast qualifier and as a qualifier of the array element type. It can result in a compiler crash, because such QualType does not compare equal to the same type but without extra qualification. To avoid the crash, the redundant qualifiers are removed while searching for common sugar. It fixes llvm#97005 (Clang crashed in ASTContext::getCommonSugaredType).

llvmbot · 2025-03-17T17:39:46Z

@llvm/pr-subscribers-clang

Author: Serge Pavlov (spavloff)

Changes

Const-qualification of an array caused by constexpr specifier can produce QualType, where the const qualifier is set both as fast qualifier and as a qualifier of the array element type. It can result in a compiler crash, because such QualType does not compare equal to the same type but without extra qualification.

To avoid the crash, the redundant qualifiers are removed while searching for common sugar.

It fixes #97005 (Clang crashed in ASTContext::getCommonSugaredType).

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

2 Files Affected:

(modified) clang/lib/AST/ASTContext.cpp (+9)
(added) clang/test/SemaCXX/constexpr-implicit-const-97005.cpp (+10)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 7ed5b033d9bd8..2614c4235c994 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -14171,6 +14171,15 @@ static QualType getCommonSugarTypeNode(ASTContext &Ctx, const Type *X,
 static auto unwrapSugar(SplitQualType &T, Qualifiers &QTotal) {
   SmallVector<SplitQualType, 8> R;
   while (true) {
+    if (const auto *ATy = dyn_cast<ArrayType>(T.Ty)) {
+      // C++ 9.3.3.4p3: Any type of the form "cv-qualifier-seq array of N U" is
+      // adjusted to "array of N cv-qualifier-seq U".
+      // C23 6.7.3p10: If the specification of an array type includes any type
+      // qualifiers, both the array and the element type are so-qualified.
+      //
+      // To simplify comparison remove the redundant qualifiers from the array.
+      T.Quals.removeCVRQualifiers(Qualifiers::Const | Qualifiers::Volatile);
+    }
     QTotal.addConsistentQualifiers(T.Quals);
     QualType NT = T.Ty->getLocallyUnqualifiedSingleStepDesugaredType();
     if (NT == QualType(T.Ty, 0))
diff --git a/clang/test/SemaCXX/constexpr-implicit-const-97005.cpp b/clang/test/SemaCXX/constexpr-implicit-const-97005.cpp
new file mode 100644
index 0000000000000..db6106998d02b
--- /dev/null
+++ b/clang/test/SemaCXX/constexpr-implicit-const-97005.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu %s
+
+bool a;
+constexpr const unsigned char c[] = { 5 };
+constexpr const unsigned char d[1] = { 0 };
+auto b = (a ? d : c);
+
+constexpr const unsigned char c1[][1] = {{ 5 }};
+constexpr const unsigned char d1[1][1] = {{ 0 }};
+auto b1 = (a ? d1 : c1);

efriedma-quic · 2025-03-17T18:39:49Z

clang/lib/AST/ASTContext.cpp

+      // qualifiers, both the array and the element type are so-qualified.
+      //
+      // To simplify comparison remove the redundant qualifiers from the array.
+      T.Quals.removeCVRQualifiers(Qualifiers::Const | Qualifiers::Volatile);


Tossing the const qualifier is fine if the array element type is already const-qualified... but do we actually know it is at this point? Or do we need something like T.Ty = getAsArrayType(getQualifiedType(T)) to make sure the qualifiers get preserved?

In the updated version, only qualifiers found in element type are removed.

mizvekov · 2025-03-17T18:51:23Z

The test for this should actually go in clang/test/SemaCXX/sugar-common-types.cpp, following the style in the file.

One example test, without using constexpr: https://godbolt.org/z/8PGa4aTPx

enum class Z {};

using ConstInt = const int;
using ConstIntArray1 = ConstInt[1];
using ConstIntArrayInc = ConstInt[];

const ConstIntArray1 c = { 5 };
const ConstIntArrayInc d = { 0 };
Z t1 = false ? d : c;

Also I note there is a separate bug here in the type printer: https://godbolt.org/z/5afcxnor7

We get a diagnostic: error: cannot initialize a variable of type 'Z' with an lvalue of type 'const ConstIntArray1' (aka 'const const int[1]'). We can drop that redundant extra const, because otherwise it does not round trip to that doubly qualified type anyway.

About the fix, I think it's at the wrong level and we shouldn't just be dropping the qualifiers like that, but it's been a while I implemented this functionality and I would need some time to think about it.

mizvekov · 2025-03-17T19:18:22Z

Regarding the double const , I think this is not a type printer bug per se.

By indiscriminately adding a qualifier to a type, we can produce a type which cannot be written, such as this const qualified array case. As such, we can never do a good job printing these anyway.

There are two issues:

A single step desugaring with qualifier preservation could cause this. This could be produced as part of the type 'aka' in diagnostics. We probably should add a special case for arrays when single step desugaring.
When adding const for constexpr declarations, we could probably add some new kind of type sugar node which represents how this qualifier was added.

mizvekov · 2025-03-17T20:48:46Z

clang/lib/AST/ASTContext.cpp

+    if (const auto *ATy = dyn_cast<ArrayType>(T.Ty)) {
+      // C++ 9.3.3.4p3: Any type of the form "cv-qualifier-seq array of N U" is
+      // adjusted to "array of N cv-qualifier-seq U".
+      // C23 6.7.3p10: If the specification of an array type includes any type
+      // qualifiers, both the array and the element type are so-qualified.
+      //
+      // To simplify comparison remove the redundant qualifiers from the array.
+      T.Quals.removeCVRQualifiers(Qualifiers::Const | Qualifiers::Volatile);


This is at the wrong level to fix the problem.
We have the logic for dealing with Array qualifiers in getCommonArrayElementType.

That's where we adjust the outer qualifiers for Array types accordingly to the qualifiers of the element type.

Looks like there is a bug in the qualifier algebra there:

QX += EX.getQualifiers() - RQ; QY += EY.getQualifiers() - RQ;

We want to break down those two lines to:

QX += EX.getQualifiers(); QY += EY.getQualifiers(); QX -= RQ; QY -= RQ;

I believe this should fix your problem.

This change makes sense, it can fix te case of "array of arry", but it cannot fix the case from #97005:

bool a; constexpr const unsigned char c[] = { 5 }; constexpr const unsigned char d[1] = { 0 }; auto b = (a ? d : c);

Here types of d and d do not contain sugar, corresponding canonical types are identical, getCommonNonSugarTypeNode is not called and getCommonArrayElementType is not called. However their types are not equal and assert(QX == QY) fails.

Alright, this needs more thought, but regardless, getCommonArrayElementType is the correct place to fix the problem.

We don't want to add special cases for ArrayTypes in unwrapSugar, because it's in the fast path, and most type nodes are not ArrayTypes, and most types don't even have ArrayTypes in them.

So this is why things are layered this way.

mizvekov · 2025-03-17T20:50:28Z

clang/test/SemaCXX/constexpr-implicit-const-97005.cpp

+// RUN: %clang_cc1 -triple x86_64-linux-gnu %s
+
+bool a;
+constexpr const unsigned char c[] = { 5 };
+constexpr const unsigned char d[1] = { 0 };
+auto b = (a ? d : c);
+
+constexpr const unsigned char c1[][1] = {{ 5 }};
+constexpr const unsigned char d1[1][1] = {{ 0 }};
+auto b1 = (a ? d1 : c1);


Please move this test to clang/test/SemaCXX/sugar-common-types.cpp, adjusting according to the function and style of that file.

Ie we want to make sure the inner sugar is preserved in the output.

The test has been moved.

- Now only qualifiers that are found in array element are cleared, - Test are moved into a new location.

github-actions · 2025-03-19T17:01:48Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 3f62718c4a907125af31faa62365bdf11ddef7b6 9bc13aee5775af148941826b74e5c9fbaab907c7 --extensions cpp -- clang/lib/AST/ASTContext.cpp clang/test/SemaCXX/sugar-common-types.cpp

View the diff from clang-format here.

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 28f4332162..5429cdc704 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -14171,7 +14171,7 @@ static QualType getCommonSugarTypeNode(ASTContext &Ctx, const Type *X,
 /// Returns element type of the array, probably multidimensional, specified by
 /// the given ArrayType.
 /// \returns Canonical type of the array element.
-static QualType getNonArrayElementType(const ArrayType* ATy) {
+static QualType getNonArrayElementType(const ArrayType *ATy) {
   QualType ElTy = ATy->getElementType().getCanonicalType();
   while (auto *SubArr = dyn_cast<ArrayType>(ElTy.getTypePtr()))
     ElTy = SubArr->getElementType();

mizvekov · 2025-03-22T17:53:57Z

I posted an alternative fix for this here: #132559

spavloff · 2025-03-26T07:02:08Z

Fixed in #132559.

spavloff requested review from mizvekov, rjmccall, AaronBallman, Fznamznon and efriedma-quic March 17, 2025 17:39

spavloff self-assigned this Mar 17, 2025

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

spavloff mentioned this pull request Mar 17, 2025

Move const qualification of array to its elements #131366

Closed

efriedma-quic reviewed Mar 17, 2025

View reviewed changes

mizvekov requested changes Mar 17, 2025

View reviewed changes

Address review comments

9bc13ae

- Now only qualifiers that are found in array element are cleared, - Test are moved into a new location.

spavloff closed this Mar 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Fix array types comparison in getCommonSugaredType #131649

[clang] Fix array types comparison in getCommonSugaredType #131649

spavloff commented Mar 17, 2025

llvmbot commented Mar 17, 2025

efriedma-quic Mar 17, 2025

spavloff Mar 19, 2025

mizvekov commented Mar 17, 2025

mizvekov commented Mar 17, 2025

mizvekov Mar 17, 2025

spavloff Mar 19, 2025

mizvekov Mar 19, 2025

mizvekov Mar 17, 2025

spavloff Mar 19, 2025

github-actions bot commented Mar 19, 2025

mizvekov commented Mar 22, 2025

spavloff commented Mar 26, 2025

[clang] Fix array types comparison in getCommonSugaredType #131649

[clang] Fix array types comparison in getCommonSugaredType #131649

Conversation

spavloff commented Mar 17, 2025

llvmbot commented Mar 17, 2025

efriedma-quic Mar 17, 2025

Choose a reason for hiding this comment

spavloff Mar 19, 2025

Choose a reason for hiding this comment

mizvekov commented Mar 17, 2025

mizvekov commented Mar 17, 2025

mizvekov Mar 17, 2025

Choose a reason for hiding this comment

spavloff Mar 19, 2025

Choose a reason for hiding this comment

mizvekov Mar 19, 2025

Choose a reason for hiding this comment

mizvekov Mar 17, 2025

Choose a reason for hiding this comment

spavloff Mar 19, 2025

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2025

mizvekov commented Mar 22, 2025

spavloff commented Mar 26, 2025