Skip to content

Conversation

@Endilll
Copy link
Contributor

@Endilll Endilll commented Oct 26, 2023

This patch adds clang::preferred_type annotations to Type-related bit-fields. At the moment only debug info takes advantage of this annotation. See more in #69104

This patch also propagates underlying type of several enums from bit-field declaration to enum declaration. I don't see how having them diverge helps.

@Endilll Endilll added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Oct 26, 2023
@Endilll Endilll requested a review from AaronBallman October 26, 2023 16:31
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Oct 26, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 26, 2023

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)

Changes

This patch adds clang::preferred_type annotations to Type-related bit-fields where possible. Notable exception are bit-fields that hold values of types that are incomplete at bit-field declaration. This is a limitation of clang::preferred_type attribute that is hard to overcome. At the moment only debug info takes advantage of this annotation. See more in #69104

This patch also propagates underlying type of several enums from bit-field declaration to enum declaration. I don't see how having them diverge helps.


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

4 Files Affected:

  • (modified) clang/include/clang/AST/DeclBase.h (+1-1)
  • (modified) clang/include/clang/AST/DependenceFlags.h (+1-1)
  • (modified) clang/include/clang/AST/Type.h (+49-45)
  • (modified) clang/include/clang/Basic/Linkage.h (+1-1)
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index 978e4255e877ec2..0307691fdd480bf 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -49,7 +49,7 @@ class ExternalSourceSymbolAttr;
 class FunctionDecl;
 class FunctionType;
 class IdentifierInfo;
-enum Linkage : unsigned char;
+enum Linkage : unsigned;
 class LinkageSpecDecl;
 class Module;
 class NamedDecl;
diff --git a/clang/include/clang/AST/DependenceFlags.h b/clang/include/clang/AST/DependenceFlags.h
index 3b3c1afb096addd..e91b6ff35b34966 100644
--- a/clang/include/clang/AST/DependenceFlags.h
+++ b/clang/include/clang/AST/DependenceFlags.h
@@ -49,7 +49,7 @@ struct ExprDependenceScope {
 using ExprDependence = ExprDependenceScope::ExprDependence;
 
 struct TypeDependenceScope {
-  enum TypeDependence : uint8_t {
+  enum TypeDependence : unsigned {
     /// Whether this type contains an unexpanded parameter pack
     /// (for C++11 variadic templates)
     UnexpandedPack = 1,
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 1e8e1303e65f6ba..55558a22ecc1b1c 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -1611,23 +1611,24 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     template <class T> friend class TypePropertyCache;
 
     /// TypeClass bitfield - Enum that specifies what subclass this belongs to.
-    unsigned TC : 8;
+    [[clang::preferred_type(TypeClass)]] unsigned TC : 8;
 
     /// Store information on the type dependency.
-    unsigned Dependence : llvm::BitWidth<TypeDependence>;
+    [[clang::preferred_type(TypeDependence)]] unsigned Dependence
+        : llvm::BitWidth<TypeDependence>;
 
     /// True if the cache (i.e. the bitfields here starting with
     /// 'Cache') is valid.
-    mutable unsigned CacheValid : 1;
+    [[clang::preferred_type(bool)]] mutable unsigned CacheValid : 1;
 
     /// Linkage of this type.
-    mutable unsigned CachedLinkage : 3;
+    [[clang::preferred_type(Linkage)]] mutable unsigned CachedLinkage : 3;
 
     /// Whether this type involves and local or unnamed types.
-    mutable unsigned CachedLocalOrUnnamed : 1;
+    [[clang::preferred_type(bool)]] mutable unsigned CachedLocalOrUnnamed : 1;
 
     /// Whether this type comes from an AST file.
-    mutable unsigned FromAST : 1;
+    [[clang::preferred_type(bool)]] mutable unsigned FromAST : 1;
 
     bool isCacheValid() const {
       return CacheValid;
@@ -1652,11 +1653,11 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
   class ArrayTypeBitfields {
     friend class ArrayType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// CVR qualifiers from declarations like
     /// 'int X[static restrict 4]'. For function parameters only.
-    unsigned IndexTypeQuals : 3;
+    [[clang::preferred_type(Qualifiers)]] unsigned IndexTypeQuals : 3;
 
     /// Storage class qualifiers from declarations like
     /// 'int X[static restrict 4]'. For function parameters only.
@@ -1671,13 +1672,13 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     unsigned : NumArrayTypeBits;
 
     /// Whether we have a stored size expression.
-    unsigned HasStoredSizeExpr : 1;
+    [[clang::preferred_type(bool)]] unsigned HasStoredSizeExpr : 1;
   };
 
   class BuiltinTypeBitfields {
     friend class BuiltinType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// The kind (BuiltinType::Kind) of builtin type this is.
     static constexpr unsigned NumOfBuiltinTypeBits = 9;
@@ -1691,16 +1692,16 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     friend class FunctionProtoType;
     friend class FunctionType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// Extra information which affects how the function is called, like
     /// regparm and the calling convention.
-    unsigned ExtInfo : 13;
+    [[clang::preferred_type(CallingConv)]] unsigned ExtInfo : 13;
 
     /// The ref-qualifier associated with a \c FunctionProtoType.
     ///
     /// This is a value of type \c RefQualifierKind.
-    unsigned RefQualifier : 2;
+    [[clang::preferred_type(RefQualifierKind)]] unsigned RefQualifier : 2;
 
     /// Used only by FunctionProtoType, put here to pack with the
     /// other bitfields.
@@ -1708,9 +1709,10 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     ///
     /// C++ 8.3.5p4: The return type, the parameter type list and the
     /// cv-qualifier-seq, [...], are part of the function type.
-    unsigned FastTypeQuals : Qualifiers::FastWidth;
+    [[clang::preferred_type(Qualifiers)]] unsigned FastTypeQuals
+        : Qualifiers::FastWidth;
     /// Whether this function has extended Qualifiers.
-    unsigned HasExtQuals : 1;
+    [[clang::preferred_type(bool)]] unsigned HasExtQuals : 1;
 
     /// The number of parameters this function has, not counting '...'.
     /// According to [implimits] 8 bits should be enough here but this is
@@ -1719,25 +1721,26 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     unsigned NumParams : 16;
 
     /// The type of exception specification this function has.
-    unsigned ExceptionSpecType : 4;
+    [[clang::preferred_type(
+        ExceptionSpecificationType)]] unsigned ExceptionSpecType : 4;
 
     /// Whether this function has extended parameter information.
-    unsigned HasExtParameterInfos : 1;
+    [[clang::preferred_type(bool)]] unsigned HasExtParameterInfos : 1;
 
     /// Whether this function has extra bitfields for the prototype.
-    unsigned HasExtraBitfields : 1;
+    [[clang::preferred_type(bool)]] unsigned HasExtraBitfields : 1;
 
     /// Whether the function is variadic.
-    unsigned Variadic : 1;
+    [[clang::preferred_type(bool)]] unsigned Variadic : 1;
 
     /// Whether this function has a trailing return type.
-    unsigned HasTrailingReturn : 1;
+    [[clang::preferred_type(bool)]] unsigned HasTrailingReturn : 1;
   };
 
   class ObjCObjectTypeBitfields {
     friend class ObjCObjectType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// The number of type arguments stored directly on this object type.
     unsigned NumTypeArgs : 7;
@@ -1746,13 +1749,13 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     unsigned NumProtocols : 6;
 
     /// Whether this is a "kindof" type.
-    unsigned IsKindOf : 1;
+    [[clang::preferred_type(bool)]] unsigned IsKindOf : 1;
   };
 
   class ReferenceTypeBitfields {
     friend class ReferenceType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// True if the type was originally spelled with an lvalue sigil.
     /// This is never true of rvalue references but can also be false
@@ -1765,17 +1768,17 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     ///   ref &&a;             // lvalue, inner ref
     ///   rvref &a;            // lvalue, inner ref, spelled lvalue
     ///   rvref &&a;           // rvalue, inner ref
-    unsigned SpelledAsLValue : 1;
+    [[clang::preferred_type(bool)]] unsigned SpelledAsLValue : 1;
 
     /// True if the inner type is a reference type.  This only happens
     /// in non-canonical forms.
-    unsigned InnerRef : 1;
+    [[clang::preferred_type(bool)]] unsigned InnerRef : 1;
   };
 
   class TypeWithKeywordBitfields {
     friend class TypeWithKeyword;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// An ElaboratedTypeKeyword.  8 bits for efficient access.
     unsigned Keyword : 8;
@@ -1786,18 +1789,18 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
   class ElaboratedTypeBitfields {
     friend class ElaboratedType;
 
-    unsigned : NumTypeBits;
-    unsigned : NumTypeWithKeywordBits;
+    [[clang::preferred_type(TypeWithKeywordBitfields)]] unsigned
+        : NumTypeWithKeywordBits;
 
     /// Whether the ElaboratedType has a trailing OwnedTagDecl.
-    unsigned HasOwnedTagDecl : 1;
+    [[clang::preferred_type(bool)]] unsigned HasOwnedTagDecl : 1;
   };
 
   class VectorTypeBitfields {
     friend class VectorType;
     friend class DependentVectorType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// The kind of vector, either a generic vector type or some
     /// target-specific vector type such as for AltiVec or Neon.
@@ -1809,7 +1812,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
   class AttributedTypeBitfields {
     friend class AttributedType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// An AttributedType::Kind
     unsigned AttrKind : 32 - NumTypeBits;
@@ -1818,11 +1821,11 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
   class AutoTypeBitfields {
     friend class AutoType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// Was this placeholder type spelled as 'auto', 'decltype(auto)',
     /// or '__auto_type'?  AutoTypeKeyword value.
-    unsigned Keyword : 2;
+    [[clang::preferred_type(AutoTypeKeyword)]] unsigned Keyword : 2;
 
     /// The number of template arguments in the type-constraints, which is
     /// expected to be able to hold at least 1024 according to [implimits].
@@ -1838,34 +1841,35 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
     friend class TypeOfType;
     friend class TypeOfExprType;
 
-    unsigned : NumTypeBits;
-    unsigned IsUnqual : 1; // If true: typeof_unqual, else: typeof
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
+    [[clang::preferred_type(
+        bool)]] unsigned IsUnqual : 1; // If true: typeof_unqual, else: typeof
   };
 
   class UsingBitfields {
     friend class UsingType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// True if the underlying type is different from the declared one.
-    unsigned hasTypeDifferentFromDecl : 1;
+    [[clang::preferred_type(bool)]] unsigned hasTypeDifferentFromDecl : 1;
   };
 
   class TypedefBitfields {
     friend class TypedefType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// True if the underlying type is different from the declared one.
-    unsigned hasTypeDifferentFromDecl : 1;
+    [[clang::preferred_type(bool)]] unsigned hasTypeDifferentFromDecl : 1;
   };
 
   class SubstTemplateTypeParmTypeBitfields {
     friend class SubstTemplateTypeParmType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
-    unsigned HasNonCanonicalUnderlyingType : 1;
+    [[clang::preferred_type(bool)]] unsigned HasNonCanonicalUnderlyingType : 1;
 
     // The index of the template parameter this substitution represents.
     unsigned Index : 15;
@@ -1881,7 +1885,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
   class SubstTemplateTypeParmPackTypeBitfields {
     friend class SubstTemplateTypeParmPackType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     // The index of the template parameter this substitution represents.
     unsigned Index : 16;
@@ -1896,10 +1900,10 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
   class TemplateSpecializationTypeBitfields {
     friend class TemplateSpecializationType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// Whether this template specialization type is a substituted type alias.
-    unsigned TypeAlias : 1;
+    [[clang::preferred_type(bool)]] unsigned TypeAlias : 1;
 
     /// The number of template arguments named in this class template
     /// specialization, which is expected to be able to hold at least 1024
@@ -1929,7 +1933,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
   class PackExpansionTypeBitfields {
     friend class PackExpansionType;
 
-    unsigned : NumTypeBits;
+    [[clang::preferred_type(TypeBitfields)]] unsigned : NumTypeBits;
 
     /// The number of expansions that this pack expansion will
     /// generate when substituted (+1), which is expected to be able to
diff --git a/clang/include/clang/Basic/Linkage.h b/clang/include/clang/Basic/Linkage.h
index 0b7b61954a075ae..bfe750462cc70b7 100644
--- a/clang/include/clang/Basic/Linkage.h
+++ b/clang/include/clang/Basic/Linkage.h
@@ -20,7 +20,7 @@ namespace clang {
 
 /// Describes the different kinds of linkage
 /// (C++ [basic.link], C99 6.2.2) that an entity may have.
-enum Linkage : unsigned char {
+enum Linkage : unsigned {
   /// No linkage, which means that the entity is unique and
   /// can only be referred to from within its scope.
   NoLinkage = 0,

This reverts commit 21689b5.
@github-actions
Copy link

github-actions bot commented Oct 26, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@Endilll
Copy link
Contributor Author

Endilll commented Oct 26, 2023

Formatting is good. I created a PR against clang-format to convince the tool: #70360

/// Describes the different kinds of linkage
/// (C++ [basic.link], C99 6.2.2) that an entity may have.
enum Linkage : unsigned char {
enum Linkage : unsigned {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is similar to TypeDependence we discuss in another thread, except that I can't find references to this type in data member declaration in our headers, which suggests that bit-field it stored in has hard-coded width, and has nothing to do with underlying type declared here.


struct TypeDependenceScope {
enum TypeDependence : uint8_t {
enum TypeDependence : unsigned {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this change? Making Type bigger than it is can have a pretty large impact on the ressource need of clang.

Copy link
Contributor Author

@Endilll Endilll Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only place this enum is stored is bit-field I annotate in this patch. Like it was with TypeAlignment, I'm only encoding status quo that actual underlying type for this enum is unsigned. There is no impact on memory footprint, because I'm yet to see us deriving bit-field width from underlying type of enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this particular case, width of bit-field is not hardcoded, but derived from enumerators via llvm::BitWidth<TypeDependence>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a chat with @Endilll offline.
The reason for this change is that preferred_type (#69104) warns when the enumerator has a different underlying type as the bit field. https://godbolt.org/z/zz99s6M3j

This warning does seem a bit off to me - but I really like its intent.
The warning only triggers on enumerators (which probably make sense as a general warning may have too many false positives), but I think the warning is too restrictive.
If the underlying type has the same signedness and has a size equal to or smaller than the the size of the type of the bitfield, I don't think a warning should be produced.

I have convinced myself that the changes here are most likely fine (TypeDependence is (afaict) only stored in TypeBitfields, as far as i can tell, and this uses the number of bits of the greatest enumerator, which i did not initially realize).

Yet I do not like the idea that we would change the underlying type of all enums used in bitfields for the sake of that warning.

My preferred solution would be to make -Wbitfield-type less aggressive and not change the underlying types of enums is this patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preferred solution would be to make -Wbitfield-type less aggressive and not change the underlying types of enums is this patch.

Yeah, this I think is my preference.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we're reviewing #70632 should we revert the changes to underlying types from this patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm putting this on hold for a bit, and refactor enumerations in NFC commits, so that this PR lands without "holes" because some enums are not complete at the point where I put preferred_type. After that underlying type changes this PR does shouldn't be necessary anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted changes to underlying types. This should be good to go now.

@tbaederr
Copy link
Contributor

Since this is done unconditionally, doesn't this mean we now need an unreleased clang to compile? Or are [[clang::]] attributes just ignored if they are unknown?

@Endilll
Copy link
Contributor Author

Endilll commented Oct 27, 2023

@tbaederr As highlighted in #69104, this attribute doesn't change semantics of the program, so it's safe to ignore.

@tbaederr
Copy link
Contributor

Sure but clang 16 will emit a warning for them: https://godbolt.org/z/jnsc4336G - are we passing -Wno-unknown-attributes to the build?

@Endilll
Copy link
Contributor Author

Endilll commented Oct 27, 2023

That's a good point. I'm not opposed to wrap this attribute in a macro if we must. I'd like to hear from @AaronBallman on this matter.

@erichkeane
Copy link
Collaborator

We definitely need to wrap these in a macro/hide them in a macro somehow. This will fail -Werror on GCC/MSVC/Older Clang compilers.

@AaronBallman
Copy link
Collaborator

We definitely need to wrap these in a macro/hide them in a macro somehow. This will fail -Werror on GCC/MSVC/Older Clang compilers.

+1, the macro would live in https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Compiler.h along with the other attribute macros.

This is made possible by several NFC commits that made some enums complete at the point of declaration of bit-fields.
Endilll added a commit that referenced this pull request Nov 2, 2023
…70632)

#69104 introduce a diagnostic that checked underlying type of an enum against type of bit-field that is annotated with `[[clang::preferred_type]]`. When I tried to introduce this annotation in #70349, it turned out to be too chatty, despite effort to avoid that.
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from a nit with the new macro (feel free to fix when landing).


/// \macro LLVM_PREFERRED_TYPE
/// Adjust type of bit-field in debug info.
#if __has_attribute(preferred_type)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if __has_attribute(preferred_type)
#if LLVM_HAS_CPP_ATTRIBUTE(clang::preferred_type)

or leave the condition as-is and change the expansion to using __attribute__(()) instead. Just making sure they both match up. (If we're using this from C interfaces, then we probably want to use the GNU-style spelling.)

@Endilll Endilll merged commit 8a3e4b5 into llvm:main Nov 2, 2023
@Endilll Endilll deleted the annotate-type-with-preferred-type branch November 2, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category llvm:support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants