[Clang] NFCI: Add a new bit HasExtraBitfields to FunctionType.

[sdesmalen]

Summary

The FunctionTypeExtraBitfields is currently only available when the
ExceptionSpecificationType == Dynamic, which means that there is no other
way to use or extend the FunctionTypeExtraBitfields independently of the
exception specification type.

This patch adds a new field HasExtraBitfields to specify
whether the prototype has trailing ExtraBitfields.

This patch intends to be NFC and is required for future extension
and use of the ExtraBitfields struct.

[sdesmalen]

This patch follows from some discussion on D124998 as well as the need for more bits to represent future function type attributes :)

[erichkeane]

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here. Also, the 'hasBitfields' seems like the answer should just be 'no' in the case when its 'no'...

clang/include/clang/AST/Type.h

4103

Why is asking if we DO have extra bitfields an assert here? I would think this is a valid question...

Why would 'dynamic' and extra-bitfields be exclusive here?

clang/lib/AST/Type.cpp

3219

this else should have braces, since the 'if' above does. (nit).

[aaron.ballman]

Thanks for this! I'm still thinking about the design for this, but spotted a few minor things that could be addressed in the meantime.

clang/include/clang/AST/Type.h

3801–3802

It looks like this comment is now out of date.

3810

More forward-looking given that NumExceptionType really should be an 8-bit wide bit-field.

[sdesmalen]

In D126642#3547284, @erichkeane wrote:

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here.

The reason I repurposed the bit was because I previously tried adding a bit to unsigned ExtInfo : 13; but that fails

static_assert(sizeof(*this) <= 8 + sizeof(ExtQualsTypeCommonBase), "changing bitfields changed sizeof(Type)!");

so perhaps incorrectly I assumed I couldn't add any new bits to FunctionType and thought I'd repurpose this one bit, because it's only ever used for FunctionProtoType (never for FunctionNoProtoType).

But I now realised I can just add an extra bit, so that we can leave this bit as-is. What do you think?

clang/include/clang/AST/Type.h

4103

This assert is merely confirming that HasExtraBitfields must be true if the ExceptionSpecType is EST_Dynamic, because that was the old behaviour (and I wanted to avoid introducing failures if some code still assumed that hasExtraBitfields == true, but for some reason HasExtraBitfields had not yet been set to true).

[erichkeane]

In D126642#3547496, @sdesmalen wrote:

In D126642#3547284, @erichkeane wrote:

I'm not sure I'm grokking hte difference between the ExtraBitfields and ExtParamInfos here.

The reason I repurposed the bit was because I previously tried adding a bit to unsigned ExtInfo : 13; but that fails

static_assert(sizeof(*this) <= 8 + sizeof(ExtQualsTypeCommonBase), "changing bitfields changed sizeof(Type)!");

Right, yeah. One thing to consider: ExtInfo was 'first', and so it got to keep the 'in bitfield bits'. However, much of what is in there is, IMO, not something that is used often enough to be worth having in there. Of the bits being used there, I think 'NoReturn' is the only one used with any regularity (other than perhaps 'this call' in Windows-32-bit machines). I wonder if we should consider moving as much of that as possible over.

so perhaps incorrectly I assumed I couldn't add any new bits to FunctionType and thought I'd repurpose this one bit, because it's only ever used for FunctionProtoType (never for FunctionNoProtoType).

But I now realised I can just add an extra bit, so that we can leave this bit as-is. What do you think?

I think there is probably value to that, yes.

[sdesmalen]

Instead of repurposing the HasExtParameterInfos bit, added an extra field for HasExtraBitfields.

[sdesmalen]

In D126642#3547526, @erichkeane wrote:

Right, yeah. One thing to consider: ExtInfo was 'first', and so it got to keep the 'in bitfield bits'. However, much of what is in there is, IMO, not something that is used often enough to be worth having in there. Of the bits being used there, I think 'NoReturn' is the only one used with any regularity (other than perhaps 'this call' in Windows-32-bit machines). I wonder if we should consider moving as much of that as possible over.

That sounds sensible. Some of the fields there are also valid in FunctionNoProtoType though, and I don't believe I saw an equivalent to ExtProtoInfo for FunctionNoProtoType, is that because it's uncommon enough that it only requires changing in a handful of places? I'm not too familiar with Clang code, so I didn't look to deep into this.

so perhaps incorrectly I assumed I couldn't add any new bits to FunctionType and thought I'd repurpose this one bit, because it's only ever used for FunctionProtoType (never for FunctionNoProtoType).

But I now realised I can just add an extra bit, so that we can leave this bit as-is. What do you think?

I think there is probably value to that, yes.

Great, thanks, I've updated that now!

clang/include/clang/AST/Type.h

3801–3802

Good spot, thanks!

4103

I've marked the comment as done hoping that my above explanation clarified it, but let me know if you're not happy with it.

[erichkeane]

In D126642#3547905, @sdesmalen wrote:

In D126642#3547526, @erichkeane wrote:

Right, yeah. One thing to consider: ExtInfo was 'first', and so it got to keep the 'in bitfield bits'. However, much of what is in there is, IMO, not something that is used often enough to be worth having in there. Of the bits being used there, I think 'NoReturn' is the only one used with any regularity (other than perhaps 'this call' in Windows-32-bit machines). I wonder if we should consider moving as much of that as possible over.

That sounds sensible. Some of the fields there are also valid in FunctionNoProtoType though, and I don't believe I saw an equivalent to ExtProtoInfo for FunctionNoProtoType, is that because it's uncommon enough that it only requires changing in a handful of places? I'm not too familiar with Clang code, so I didn't look to deep into this.

Ah, hrmph, yeah, those have to be available anywhere that a function type is defined, since they are part of the no-prototype type of the function. So we'd likely need to extract that at the "FunctionType" level, and put it into trailing storage in BOTH places (that is, FunctionNoProtoType would have trailing storage for it if necessary, and FunctionProtoType would as well). ExtProtoInfo doesn't exist for FunctionNoProtoType because it is C++/parameter-specific stuff (prototypeless function types aren't permitted in C++).

so perhaps incorrectly I assumed I couldn't add any new bits to FunctionType and thought I'd repurpose this one bit, because it's only ever used for FunctionProtoType (never for FunctionNoProtoType).

But I now realised I can just add an extra bit, so that we can leave this bit as-is. What do you think?

I think there is probably value to that, yes.

Great, thanks, I've updated that now!

[erichkeane]

I think I'm Ok with this as a 1st step for the cleanup. I think we should probably evaluate what amount of work is necessary to extract the ExtInfo out into trailing storage, depending on whether it saves us room in the FunctionProtoType and FunctionNoProtoType types.

clang/include/clang/AST/Type.h

4103

Ah, that makes sense, thanks.

[aaron.ballman]

LGTM aside from a nit.

clang/include/clang/AST/Type.h

3807–3814

[sdesmalen]

Thanks for the review and comments @erichkeane and @aaron.ballman!

clang/include/clang/AST/Type.h

class FunctionTypeBitfields {
     /// According to [implimits] 8 bits should be enough here but this is
     /// somewhat easy to exceed with metaprogramming and so we would like to
     /// keep NumParams as wide as reasonably possible.
     unsigned NumParams : 16;
 
     /// The type of exception specification this function has.
     unsigned ExceptionSpecType : 4;
 
-    /// Whether this function has extended parameter information.
-    unsigned HasExtParameterInfos : 1;
+    /// Whether this function has extra bitfields for the prototype.
+    unsigned HasExtraBitfields : 1;
 
     /// Whether the function is variadic.
     unsigned Variadic : 1;
 
     /// Whether this function has a trailing return type.
     unsigned HasTrailingReturn : 1;
   };
 
public:
 
   /// A simple holder for a QualType representing a type in an
   /// exception specification. Unfortunately needed by FunctionProtoType
   /// because TrailingObjects cannot handle repeated types.
   struct ExceptionType { QualType Type; };
 
   /// A simple holder for various uncommon bits which do not fit in
   /// FunctionTypeBitfields. Aligned to alignof(void *) to maintain the
   /// alignment of subsequent objects in TrailingObjects. You must update
   /// hasExtraBitfields in FunctionProtoType after adding extra data here.
   struct alignas(void *) FunctionTypeExtraBitfields {
     /// The number of types in the exception specification.
     /// A whole unsigned is not needed here and according to
     /// [implimits] 8 bits would be enough here.
     unsigned NumExceptionType;
+
+    /// Whether this function has extended parameter information.
+    bool HasExtParameterInfos : 1;
+
+    FunctionTypeExtraBitfields()
+        : NumExceptionType(0), HasExtParameterInfos(false) {}
   };
 
 protected:
   FunctionType(TypeClass tc, QualType res, QualType Canonical,
                TypeDependence Dependence, ExtInfo Info)
       : Type(tc, Canonical, Dependence), ResultType(res) {
     FunctionTypeBits.ExtInfo = Info.Bits;
   }
 
struct ExtProtoInfo {
     ExtProtoInfo(CallingConv CC)
         : ExtInfo(CC), Variadic(false), HasTrailingReturn(false) {}
 
     ExtProtoInfo withExceptionSpec(const ExceptionSpecInfo &ESI) {
       ExtProtoInfo Result(*this);
       Result.ExceptionSpec = ESI;
       return Result;
     }
+
+    bool requiresFunctionProtoTypeExtraBitfields() const {
+      return ExtParameterInfos != nullptr || ExceptionSpec.Type == EST_Dynamic;
+    }
   };
 
 private:
   unsigned numTrailingObjects(OverloadToken<QualType>) const {
     return getNumParams();
   }
 
   unsigned numTrailingObjects(OverloadToken<SourceLocation>) const {
private:
 
   /// Return the number and kind of trailing objects
   /// related to the exception specification.
   ExceptionSpecSizeHolder getExceptionSpecSize() const {
     return getExceptionSpecSize(getExceptionSpecType(), getNumExceptions());
   }
 
   /// Whether the trailing FunctionTypeExtraBitfields is present.
-  static bool hasExtraBitfields(ExceptionSpecificationType EST) {
-    // If the exception spec type is EST_Dynamic then we have > 0 exception
-    // types and the exact number is stored in FunctionTypeExtraBitfields.
-    return EST == EST_Dynamic;
-  }
-
-  /// Whether the trailing FunctionTypeExtraBitfields is present.
   bool hasExtraBitfields() const {
-    return hasExtraBitfields(getExceptionSpecType());
+    assert((getExceptionSpecType() != EST_Dynamic ||
+            FunctionTypeBits.HasExtraBitfields) &&
+           "ExtraBitfields are required for given ExceptionSpecType");
+    return FunctionTypeBits.HasExtraBitfields;
+
   }
 
   bool hasExtQualifiers() const {
     return FunctionTypeBits.HasExtQuals;
   }
 
 public:
   unsigned getNumParams() const { return FunctionTypeBits.NumParams; }
public:
 
   exception_iterator exception_end() const {
     return exception_begin() + getNumExceptions();
   }
 
   /// Is there any interesting extra information for any of the parameters
   /// of this function type?
   bool hasExtParameterInfos() const {
-    return FunctionTypeBits.HasExtParameterInfos;
+    return hasExtraBitfields() &&
+           getTrailingObjects<FunctionTypeExtraBitfields>()
+               ->HasExtParameterInfos;
   }
 
   ArrayRef<ExtParameterInfo> getExtParameterInfos() const {
     assert(hasExtParameterInfos());
     return ArrayRef<ExtParameterInfo>(getTrailingObjects<ExtParameterInfo>(),
                                       getNumParams());
   }
 

clang/lib/AST/ASTContext.cpp

QualType ASTContext::getFunctionTypeInternal(
   // Compute the needed size to hold this FunctionProtoType and the
   // various trailing objects.
   auto ESH = FunctionProtoType::getExceptionSpecSize(
       EPI.ExceptionSpec.Type, EPI.ExceptionSpec.Exceptions.size());
   size_t Size = FunctionProtoType::totalSizeToAlloc<
       QualType, SourceLocation, FunctionType::FunctionTypeExtraBitfields,
       FunctionType::ExceptionType, Expr *, FunctionDecl *,
       FunctionProtoType::ExtParameterInfo, Qualifiers>(
-      NumArgs, EPI.Variadic,
-      FunctionProtoType::hasExtraBitfields(EPI.ExceptionSpec.Type),
+      NumArgs, EPI.Variadic, EPI.requiresFunctionProtoTypeExtraBitfields(),
       ESH.NumExceptionType, ESH.NumExprPtr, ESH.NumFunctionDeclPtr,
       EPI.ExtParameterInfos ? NumArgs : 0,
       EPI.TypeQuals.hasNonFastQualifiers() ? 1 : 0);
 
   auto *FTP = (FunctionProtoType *)Allocate(Size, TypeAlignment);
   FunctionProtoType::ExtProtoInfo newEPI = EPI;
   new (FTP) FunctionProtoType(ResultTy, ArgArray, Canonical, newEPI);
   Types.push_back(FTP);

clang/lib/AST/Type.cpp

FunctionProtoType::FunctionProtoType(QualType result, ArrayRef<QualType> params,
                                      const ExtProtoInfo &epi)
     : FunctionType(FunctionProto, result, canonical, result->getDependence(),
                    epi.ExtInfo) {
   FunctionTypeBits.FastTypeQuals = epi.TypeQuals.getFastQualifiers();
   FunctionTypeBits.RefQualifier = epi.RefQualifier;
   FunctionTypeBits.NumParams = params.size();
   assert(getNumParams() == params.size() && "NumParams overflow!");
   FunctionTypeBits.ExceptionSpecType = epi.ExceptionSpec.Type;
-  FunctionTypeBits.HasExtParameterInfos = !!epi.ExtParameterInfos;
   FunctionTypeBits.Variadic = epi.Variadic;
   FunctionTypeBits.HasTrailingReturn = epi.HasTrailingReturn;
 
-  // Fill in the extra trailing bitfields if present.
-  if (hasExtraBitfields(epi.ExceptionSpec.Type)) {
+  if (epi.requiresFunctionProtoTypeExtraBitfields()) {
+    FunctionTypeBits.HasExtraBitfields = true;
     auto &ExtraBits = *getTrailingObjects<FunctionTypeExtraBitfields>();
-    ExtraBits.NumExceptionType = epi.ExceptionSpec.Exceptions.size();
-  }
+    ExtraBits = FunctionTypeExtraBitfields();
+  } else
+    FunctionTypeBits.HasExtraBitfields = false;
+
 
   // Fill in the trailing argument array.
   auto *argSlot = getTrailingObjects<QualType>();
   for (unsigned i = 0; i != getNumParams(); ++i) {
     addDependence(params[i]->getDependence() &
                   ~TypeDependence::VariablyModified);
     argSlot[i] = params[i];
   }
 
   // Fill in the exception type array if present.
   if (getExceptionSpecType() == EST_Dynamic) {
+    auto &ExtraBits = *getTrailingObjects<FunctionTypeExtraBitfields>();
+    ExtraBits.NumExceptionType = epi.ExceptionSpec.Exceptions.size();
+
     assert(hasExtraBitfields() && "missing trailing extra bitfields!");
     auto *exnSlot =
         reinterpret_cast<QualType *>(getTrailingObjects<ExceptionType>());
     unsigned I = 0;
     for (QualType ExceptionType : epi.ExceptionSpec.Exceptions) {
       // Note that, before C++17, a dependent exception specification does
       // *not* make a type dependent; it's not even part of the C++ type
       // system.
if (isCanonicalUnqualified()) {
     }
   } else if (getCanonicalTypeInternal()->isDependentType()) {
     // Ask our canonical type whether our exception specification was dependent.
     addDependence(TypeDependence::DependentInstantiation);
   }
 
   // Fill in the extra parameter info if present.
   if (epi.ExtParameterInfos) {
+    auto &ExtraBits = *getTrailingObjects<FunctionTypeExtraBitfields>();
+    ExtraBits.HasExtParameterInfos = true;
+
     auto *extParamInfos = getTrailingObjects<ExtParameterInfo>();
     for (unsigned i = 0; i != getNumParams(); ++i)
       extParamInfos[i] = epi.ExtParameterInfos[i];
   }
 
   if (epi.TypeQuals.hasNonFastQualifiers()) {
     FunctionTypeBits.HasExtQuals = 1;
     *getTrailingObjects<Qualifiers>() = epi.TypeQuals;