[Clang] Limit FunctionTypeExtraBitfields::NumExceptionType to 16 bits.

[sdesmalen]

Summary

In https://reviews.llvm.org/D127762#4102578 @erichkeane suggested to
limit size of this field to 16bits, such that the field that encodes the
SME attributes for a function fall within the alignment of the struct for
32bit platforms.

Standard implimits defines the minimum handlers per try block to 256,
which suggests that 16bits should be more than sufficient for most
programs. Erich also pointed out that exception specs are being
deprecated and are rarely used, so hopefully this change is safe to make.

[tbaeder]

clang/include/clang/AST/Type.h

3956

Isn't using a uint16_t preferred over a bitfield?

[erichkeane]

clang/include/clang/AST/Type.h

3956

Ah, thats a great point, yes, I think that is probably better here! I like the assert on setting it (particularly because we're shrinking it!), but that idea makes this patch much smaller.

[sdesmalen]

clang/include/clang/AST/Type.h

3956

The reason for making this a bitfield rather than uint16_t is because D127762 extends the struct with another (bit)field:

/// Any AArch64 SME ACLE type attributes that need to be propagated
/// on declarations and function pointers.
unsigned AArch64SMEAttributes : 6;

I thought having this all be bitfields made more sense, but I'm happy to change it to a uint16_t if that is preferred.

that idea makes this patch much smaller

I suspect that the assert is still required to ensure that the value stored fits the value? Using uint16_t doesn't change that, or did I miss something? :)

[erichkeane]

clang/include/clang/AST/Type.h

3956

We don't need them all to be bitfields, they'll take up the same amount of space with out it, and save us a bit of oddity.

I like the assert still as well, but since this size is set in 1 place, an assert there would be fine as well.

[sdesmalen]

Use uint16_t instead of unsigned bitfield.
Also removed accessor methods in favour of assert at point of writing NumExceptionType.

clang/include/clang/AST/Type.h

3956

Okay that makes sense, thanks.

[erichkeane]

clang/lib/AST/Type.cpp

3374–3377

So this isn't a place where we're allowed to use auto. We can only use it if the type is listed on the RHS (like in the case of casts/etc).

[sdesmalen]

Use size_t instead of auto.

clang/lib/AST/Type.cpp

3374–3377

You're absolutely right, sorry about that!

clang/include/clang/AST/Type.h

public:
 
   /// 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.
   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 = 0;
+    uint16_t NumExceptionType = 0;
   };
 
 protected:
   FunctionType(TypeClass tc, QualType res, QualType Canonical,
                TypeDependence Dependence, ExtInfo Info)
       : Type(tc, Canonical, Dependence), ResultType(res) {
     FunctionTypeBits.ExtInfo = Info.Bits;
   }

clang/lib/AST/Type.cpp

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();
+    size_t NumExceptions = epi.ExceptionSpec.Exceptions.size();
+    assert(NumExceptions <= UINT16_MAX &&
+           "Not enough bits to encode exceptions");
+    ExtraBits.NumExceptionType = NumExceptions;
 
     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