[Clang] [C23] Fix typeof_unqual for qualified array types #92767

MitalAshok · 2024-05-20T15:09:41Z

Properly remove qualifiers for both the element type and the array type

Fixes #92667

Properly remove qualifiers for both the element type and the array type Fixes llvm#92667

llvmbot · 2024-05-20T15:10:17Z

@llvm/pr-subscribers-clang

Author: Mital Ashok (MitalAshok)

Changes

Properly remove qualifiers for both the element type and the array type

Fixes #92667

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

5 Files Affected:

(modified) clang/include/clang/AST/ASTContext.h (+5-1)
(modified) clang/include/clang/AST/Type.h (+17-20)
(modified) clang/lib/AST/ASTContext.cpp (+7-7)
(modified) clang/lib/AST/Type.cpp (+30-8)
(modified) clang/test/Sema/c2x-typeof.c (+25)

diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index e03b112194786..ff7bdb7e7e1a6 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -2611,7 +2611,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
   ///
   /// \returns if this is an array type, the completely unqualified array type
   /// that corresponds to it. Otherwise, returns T.getUnqualifiedType().
-  QualType getUnqualifiedArrayType(QualType T, Qualifiers &Quals);
+  QualType getUnqualifiedArrayType(QualType T, Qualifiers &Quals) const;
+  QualType getUnqualifiedArrayType(QualType T) const {
+    Qualifiers Quals;
+    return getUnqualifiedArrayType(T, Quals);
+  }
 
   /// Determine whether the given types are equivalent after
   /// cvr-qualifiers have been removed.
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index da3834f19ca04..df7f396bae095 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1605,6 +1605,10 @@ class QualType {
   QualType stripObjCKindOfType(const ASTContext &ctx) const;
 
   /// Remove all qualifiers including _Atomic.
+  ///
+  /// Like getUnqualifiedType(), the type may still be qualified if it is a
+  /// sugared array type.  To strip qualifiers even from within a sugared array
+  /// type, use ASTContext::getUnqualifiedArrayType.
   QualType getAtomicUnqualifiedType() const;
 
 private:
@@ -2092,8 +2096,8 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
 
     LLVM_PREFERRED_TYPE(TypeBitfields)
     unsigned : NumTypeBits;
-    LLVM_PREFERRED_TYPE(bool)
-    unsigned IsUnqual : 1; // If true: typeof_unqual, else: typeof
+    LLVM_PREFERRED_TYPE(TypeOfKind)
+    unsigned Kind : 1;
   };
 
   class UsingBitfields {
@@ -5273,19 +5277,20 @@ class MacroQualifiedType : public Type {
 /// extension) or a `typeof_unqual` expression (a C23 feature).
 class TypeOfExprType : public Type {
   Expr *TOExpr;
+  const ASTContext &Context;
 
 protected:
   friend class ASTContext; // ASTContext creates these.
 
-  TypeOfExprType(Expr *E, TypeOfKind Kind, QualType Can = QualType());
+  TypeOfExprType(const ASTContext &Context, Expr *E, TypeOfKind Kind,
+                 QualType Can = QualType());
 
 public:
   Expr *getUnderlyingExpr() const { return TOExpr; }
 
   /// Returns the kind of 'typeof' type this is.
   TypeOfKind getKind() const {
-    return TypeOfBits.IsUnqual ? TypeOfKind::Unqualified
-                               : TypeOfKind::Qualified;
+    return static_cast<TypeOfKind>(TypeOfBits.Kind);
   }
 
   /// Remove a single level of sugar.
@@ -5306,7 +5311,8 @@ class TypeOfExprType : public Type {
 class DependentTypeOfExprType : public TypeOfExprType,
                                 public llvm::FoldingSetNode {
 public:
-  DependentTypeOfExprType(Expr *E, TypeOfKind Kind) : TypeOfExprType(E, Kind) {}
+  DependentTypeOfExprType(const ASTContext &Context, Expr *E, TypeOfKind Kind)
+      : TypeOfExprType(Context, E, Kind) {}
 
   void Profile(llvm::FoldingSetNodeID &ID, const ASTContext &Context) {
     Profile(ID, Context, getUnderlyingExpr(),
@@ -5323,32 +5329,23 @@ class TypeOfType : public Type {
   friend class ASTContext; // ASTContext creates these.
 
   QualType TOType;
+  const ASTContext &Context;
 
-  TypeOfType(QualType T, QualType Can, TypeOfKind Kind)
-      : Type(TypeOf,
-             Kind == TypeOfKind::Unqualified ? Can.getAtomicUnqualifiedType()
-                                             : Can,
-             T->getDependence()),
-        TOType(T) {
-    TypeOfBits.IsUnqual = Kind == TypeOfKind::Unqualified;
-  }
+  TypeOfType(const ASTContext &Context, QualType T, QualType Can,
+             TypeOfKind Kind);
 
 public:
   QualType getUnmodifiedType() const { return TOType; }
 
   /// Remove a single level of sugar.
-  QualType desugar() const {
-    QualType QT = getUnmodifiedType();
-    return TypeOfBits.IsUnqual ? QT.getAtomicUnqualifiedType() : QT;
-  }
+  QualType desugar() const;
 
   /// Returns whether this type directly provides sugar.
   bool isSugared() const { return true; }
 
   /// Returns the kind of 'typeof' type this is.
   TypeOfKind getKind() const {
-    return TypeOfBits.IsUnqual ? TypeOfKind::Unqualified
-                               : TypeOfKind::Qualified;
+    return static_cast<TypeOfKind>(TypeOfBits.Kind);
   }
 
   static bool classof(const Type *T) { return T->getTypeClass() == TypeOf; }
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 8fc2bb8c401c2..ddad2ae355487 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -5682,19 +5682,19 @@ QualType ASTContext::getTypeOfExprType(Expr *tofExpr, TypeOfKind Kind) const {
     if (Canon) {
       // We already have a "canonical" version of an identical, dependent
       // typeof(expr) type. Use that as our canonical type.
-      toe = new (*this, alignof(TypeOfExprType))
-          TypeOfExprType(tofExpr, Kind, QualType((TypeOfExprType *)Canon, 0));
+      toe = new (*this, alignof(TypeOfExprType)) TypeOfExprType(
+          *this, tofExpr, Kind, QualType((TypeOfExprType *)Canon, 0));
     } else {
       // Build a new, canonical typeof(expr) type.
       Canon = new (*this, alignof(DependentTypeOfExprType))
-          DependentTypeOfExprType(tofExpr, Kind);
+          DependentTypeOfExprType(*this, tofExpr, Kind);
       DependentTypeOfExprTypes.InsertNode(Canon, InsertPos);
       toe = Canon;
     }
   } else {
     QualType Canonical = getCanonicalType(tofExpr->getType());
     toe = new (*this, alignof(TypeOfExprType))
-        TypeOfExprType(tofExpr, Kind, Canonical);
+        TypeOfExprType(*this, tofExpr, Kind, Canonical);
   }
   Types.push_back(toe);
   return QualType(toe, 0);
@@ -5707,8 +5707,8 @@ QualType ASTContext::getTypeOfExprType(Expr *tofExpr, TypeOfKind Kind) const {
 /// on canonical types (which are always unique).
 QualType ASTContext::getTypeOfType(QualType tofType, TypeOfKind Kind) const {
   QualType Canonical = getCanonicalType(tofType);
-  auto *tot =
-      new (*this, alignof(TypeOfType)) TypeOfType(tofType, Canonical, Kind);
+  auto *tot = new (*this, alignof(TypeOfType))
+      TypeOfType(*this, tofType, Canonical, Kind);
   Types.push_back(tot);
   return QualType(tot, 0);
 }
@@ -6093,7 +6093,7 @@ CanQualType ASTContext::getCanonicalParamType(QualType T) const {
 }
 
 QualType ASTContext::getUnqualifiedArrayType(QualType type,
-                                             Qualifiers &quals) {
+                                             Qualifiers &quals) const {
   SplitQualType splitType = type.getSplitUnqualifiedType();
 
   // FIXME: getSplitUnqualifiedType() actually walks all the way to
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index e31741cd44240..a84221233dd26 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -1617,9 +1617,10 @@ QualType QualType::stripObjCKindOfType(const ASTContext &constCtx) const {
 }
 
 QualType QualType::getAtomicUnqualifiedType() const {
-  if (const auto AT = getTypePtr()->getAs<AtomicType>())
-    return AT->getValueType().getUnqualifiedType();
-  return getUnqualifiedType();
+  QualType T = *this;
+  if (const auto AT = T.getTypePtr()->getAs<AtomicType>())
+    T = AT->getValueType();
+  return T.getUnqualifiedType();
 }
 
 std::optional<ArrayRef<QualType>>
@@ -3860,18 +3861,19 @@ QualType MacroQualifiedType::getModifiedType() const {
   return Inner;
 }
 
-TypeOfExprType::TypeOfExprType(Expr *E, TypeOfKind Kind, QualType Can)
+TypeOfExprType::TypeOfExprType(const ASTContext &Context, Expr *E,
+                               TypeOfKind Kind, QualType Can)
     : Type(TypeOfExpr,
            // We have to protect against 'Can' being invalid through its
            // default argument.
            Kind == TypeOfKind::Unqualified && !Can.isNull()
-               ? Can.getAtomicUnqualifiedType()
+               ? Context.getUnqualifiedArrayType(Can.getAtomicUnqualifiedType())
                : Can,
            toTypeDependence(E->getDependence()) |
                (E->getType()->getDependence() &
                 TypeDependence::VariablyModified)),
-      TOExpr(E) {
-  TypeOfBits.IsUnqual = Kind == TypeOfKind::Unqualified;
+      TOExpr(E), Context(Context) {
+  TypeOfBits.Kind = static_cast<unsigned>(Kind);
 }
 
 bool TypeOfExprType::isSugared() const {
@@ -3881,7 +3883,9 @@ bool TypeOfExprType::isSugared() const {
 QualType TypeOfExprType::desugar() const {
   if (isSugared()) {
     QualType QT = getUnderlyingExpr()->getType();
-    return TypeOfBits.IsUnqual ? QT.getAtomicUnqualifiedType() : QT;
+    return getKind() == TypeOfKind::Unqualified
+               ? Context.getUnqualifiedArrayType(QT.getAtomicUnqualifiedType())
+               : QT;
   }
   return QualType(this, 0);
 }
@@ -3893,6 +3897,24 @@ void DependentTypeOfExprType::Profile(llvm::FoldingSetNodeID &ID,
   ID.AddBoolean(IsUnqual);
 }
 
+TypeOfType::TypeOfType(const ASTContext &Context, QualType T, QualType Can,
+                       TypeOfKind Kind)
+    : Type(TypeOf,
+           Kind == TypeOfKind::Unqualified
+               ? Context.getUnqualifiedArrayType(Can.getAtomicUnqualifiedType())
+               : Can,
+           T->getDependence()),
+      TOType(T), Context(Context) {
+  TypeOfBits.Kind = static_cast<unsigned>(Kind);
+}
+
+QualType TypeOfType::desugar() const {
+  QualType QT = getUnmodifiedType();
+  return getKind() == TypeOfKind::Unqualified
+             ? Context.getUnqualifiedArrayType(QT.getAtomicUnqualifiedType())
+             : QT;
+}
+
 DecltypeType::DecltypeType(Expr *E, QualType underlyingType, QualType can)
     // C++11 [temp.type]p2: "If an expression e involves a template parameter,
     // decltype(e) denotes a unique dependent type." Hence a decltype type is
diff --git a/clang/test/Sema/c2x-typeof.c b/clang/test/Sema/c2x-typeof.c
index cf985c244f4a4..7da25aec4d3f6 100644
--- a/clang/test/Sema/c2x-typeof.c
+++ b/clang/test/Sema/c2x-typeof.c
@@ -92,3 +92,28 @@ extern __attribute__((address_space(0))) int type_attr_test_2;          // expec
 void invalid_param_fn(__attribute__((address_space(1))) int i); // expected-error {{parameter may not be qualified with an address space}}
 typeof(invalid_param_fn) invalid_param_1;
 typeof_unqual(invalid_param_fn) invalid_param_2;
+
+// Ensure restrict is stripped
+extern int *restrict p1;
+extern int *p2;
+extern typeof(p1) p1;
+extern typeof_unqual(p1) p2;
+
+// Ensure array qualifications are removed
+extern const int aci[2];
+extern const int acii[2][2];
+extern int ai[2];
+extern int aii[2][2];
+extern typeof(aci) aci;
+extern typeof_unqual(aci) ai;
+extern typeof(acii) acii;
+extern typeof_unqual(acii) aii;
+
+extern int *restrict arpi[2];
+extern int *restrict arpii[2][2];
+extern int *api[2];
+extern int *apii[2][2];
+extern typeof(arpi) arpi;
+extern typeof_unqual(arpi) api;
+extern typeof(arpii) arpii;
+extern typeof_unqual(arpii) apii;

AaronBallman

The changes generally LG, but there's a merge conflict preventing the PR from going through precommit CI.

clang/include/clang/AST/Type.h

clang/test/Sema/c2x-typeof.c

AaronBallman

LGTM! Precommit CI failure is unrelated to your changes.

clang/test/Sema/c2x-typeof.c

AaronBallman · 2024-05-30T20:18:10Z

clang/test/Sema/c2x-typeof.c

+extern typeof_unqual(aAi) ai;
+extern typeof(aAii) aAii;
+extern typeof_unqual(aAii) aii;


I think this is a more complicated situation, perhaps. _Atomic as a type qualifier (as with _Atomic type) is excluded in terms of arrays and elements being identically qualified (see footnote 42, which says "This does not apply to the _Atomic qualifier.") so the array is not atomic to begin with. So typeof_unqual would strip the non-existent qualifiers from the array in this case but leave them alone in the element type. However, _Atomic as a type specifier (as with _Atomic(type)) is an atomic-qualified type and so it would be stripped in that case (see footnote 146 which also reminds us that _Atomic(type-name) is an atomic-qualified type).

So I think the behavior of this case is correct but we need another test for _Atomic(int).

I don't think this is the correct behaviour now. _Atomic(int) x[2][2] and int _Atomic y[2][2] have the same type, and typeof_unqual(int _Atomic[2][2]) and typeof_unqual(_Atomic(int)[2][2]) should return the same type unchanged because, as you've said, the array isn't _Atomic qualified (and arrays cant be _Atomic in C). So I'll probably have to remove Context::getAtomicUnqualifiedArrayType and replace it with Context.getUnqualifiedArrayType(Can.getAtomicUnqualifiedType())

Ah, so yeah, I think I was confused above. Both typeof(_Atomic(int)[2][2]) and typeof(_Atomit int[2][2]) would return a two-dimensional array of atomic-qualified ints, and typeof_unqual is defined to return the non-atomic, unqualified version of the type that would come out of typeof, so that would mean both forms come out of typeof_unqual the same.

AaronBallman

LGTM thank you!

AaronBallman · 2024-07-12T16:32:23Z

Just making sure that this didn't fall off your radar; it seems like a good fix for us to get into Clang 19 before the branch, if possible.

MitalAshok · 2024-07-12T19:46:03Z

@AaronBallman was there anything else that needed to be done? Just did a merge because ReleaseNotes was edited but this should be fine

AaronBallman · 2024-07-15T14:28:15Z

@AaronBallman was there anything else that needed to be done? Just did a merge because ReleaseNotes was edited but this should be fine

Nope, this is approved to land.

cor3ntin · 2024-07-17T12:06:17Z

@MitalAshok ping

Summary: Properly remove qualifiers for both the element type and the array type Fixes #92667 --------- Co-authored-by: cor3ntin <[email protected]> Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251748

[Clang] [C23] Fix typeof_unqual for qualified array types

f87cb4c

Properly remove qualifiers for both the element type and the array type Fixes llvm#92667

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

AaronBallman self-requested a review May 20, 2024 15:29

AaronBallman added the c23 label May 20, 2024

MitalAshok added 2 commits May 20, 2024 16:38

Fix handling arrays of _Atomic types

a30591f

Add relase note

4609f91

AaronBallman reviewed May 21, 2024

View reviewed changes

clang/include/clang/AST/Type.h Show resolved Hide resolved

clang/test/Sema/c2x-typeof.c Outdated Show resolved Hide resolved

MitalAshok added 2 commits May 21, 2024 15:42

Merge branch 'main' into fix-typeof_unqual-array

866d578

Add example from issue

7ec8280

AaronBallman approved these changes May 28, 2024

View reviewed changes

AaronBallman reviewed May 30, 2024

View reviewed changes

MitalAshok added 2 commits May 30, 2024 22:08

Merge branch 'main' into fix-typeof_unqual-array

4867fbe

Don't remove _Atomic of element types of arrays which are not atomic

6192609

AaronBallman approved these changes May 31, 2024

View reviewed changes

MitalAshok added 2 commits June 29, 2024 12:10

Merge branch 'main' into fix-typeof_unqual-array

a6f3891

Merge branch 'main' into fix-typeof_unqual-array

a33b4c7

Merge branch 'main' into fix-typeof_unqual-array

216a4d4

cor3ntin added 2 commits July 17, 2024 15:10

Merge branch 'main' into fix-typeof_unqual-array

6459f50

Merge branch 'main' into fix-typeof_unqual-array

0c3a2ff

cor3ntin merged commit a56e009 into llvm:main Jul 17, 2024
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] [C23] Fix typeof_unqual for qualified array types #92767

[Clang] [C23] Fix typeof_unqual for qualified array types #92767

Uh oh!

MitalAshok commented May 20, 2024

Uh oh!

llvmbot commented May 20, 2024

Uh oh!

AaronBallman left a comment

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Uh oh!

Uh oh!

Uh oh!

AaronBallman May 30, 2024

Uh oh!

MitalAshok May 30, 2024

Uh oh!

AaronBallman May 31, 2024

Uh oh!

AaronBallman left a comment

Uh oh!

AaronBallman commented Jul 12, 2024

Uh oh!

MitalAshok commented Jul 12, 2024

Uh oh!

AaronBallman commented Jul 15, 2024

Uh oh!

cor3ntin commented Jul 17, 2024

Uh oh!

Uh oh!

Uh oh!

[Clang] [C23] Fix typeof_unqual for qualified array types #92767

[Clang] [C23] Fix typeof_unqual for qualified array types #92767

Uh oh!

Conversation

MitalAshok commented May 20, 2024

Uh oh!

llvmbot commented May 20, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AaronBallman May 30, 2024

Choose a reason for hiding this comment

Uh oh!

MitalAshok May 30, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman May 31, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Jul 12, 2024

Uh oh!

MitalAshok commented Jul 12, 2024

Uh oh!

AaronBallman commented Jul 15, 2024

Uh oh!

cor3ntin commented Jul 17, 2024

Uh oh!

Uh oh!

Uh oh!