Widen EHScope::ClenupBitFields::FixupDepth to avoid overflowing it (PR23490)

[hans]

Summary

It currently only takes 2048 gotos to overflow the FixupDepth bitfield, causing silent miscompilation. Apparently some parser generators run into this (see PR).

I don't know that that data structure is terribly size sensitive anyway, and since there's no room to widen the bitfield, let's just use a separate word for it.

[rjmccall]

Yeah, 2048 is clearly too few. The other changes aren't necessary; if we're going to try to be that generous, there's a lot of places in the AST that need to be updated.

lib/CodeGen/CGCleanup.h

61 ↗

(On Diff #61428)

This one seems unnecessary. 29 bits is fine.

93 ↗

(On Diff #61428)

This should just be pulled out of the BitFields structure and into the EHCleanup class.

100 ↗

(On Diff #61428)

This one is also unnecessary.

[hans]

Addressing comments.

[hans]

Thanks for the review! Please take another look.

lib/CodeGen/CGCleanup.h

93 ↗

(On Diff #61428)

That's probably a better way to do it. Thanks.

[rjmccall]

That looks great, thanks.

cfe/trunk/lib/CodeGen/CGCleanup.h

class CleanupBitFields {
     unsigned TestFlagInNormalCleanup : 1;
 
     /// Whether the EH cleanup should test the activation flag.
     unsigned TestFlagInEHCleanup : 1;
 
     /// The amount of extra storage needed by the Cleanup.
     /// Always a multiple of the scope-stack alignment.
     unsigned CleanupSize : 12;
-
-    /// The number of fixups required by enclosing scopes (not including
-    /// this one).  If this is the top cleanup scope, all the fixups
-    /// from this index onwards belong to this scope.
-    unsigned FixupDepth : 32 - 18 - NumCommonBits; // currently 12
   };
 
   class FilterBitFields {
     friend class EHFilterScope;
     unsigned : NumCommonBits;
 
     unsigned NumFilters : 32 - NumCommonBits;
   };
public:
   static size_t getSizeForNumHandlers(unsigned N) {
     return sizeof(EHCatchScope) + N * sizeof(Handler);
   }
 
   EHCatchScope(unsigned numHandlers,
                EHScopeStack::stable_iterator enclosingEHScope)
     : EHScope(Catch, enclosingEHScope) {
     CatchBits.NumHandlers = numHandlers;
+    assert(CatchBits.NumHandlers == numHandlers && "NumHandlers overflow?");
   }
 
   unsigned getNumHandlers() const {
     return CatchBits.NumHandlers;
   }
 
   void setCatchAllHandler(unsigned I, llvm::BasicBlock *Block) {
     setHandler(I, CatchTypeInfo{nullptr, 0}, Block);
struct ExtInfo {
     llvm::SmallPtrSet<llvm::BasicBlock*, 4> Branches;
 
     /// Normal branch-afters.
     SmallVector<std::pair<llvm::BasicBlock*,llvm::ConstantInt*>, 4>
       BranchAfters;
   };
   mutable struct ExtInfo *ExtInfo;
 
+  /// The number of fixups required by enclosing scopes (not including
+  /// this one).  If this is the top cleanup scope, all the fixups
+  /// from this index onwards belong to this scope.
+  unsigned FixupDepth;
+
   struct ExtInfo &getExtInfo() {
     if (!ExtInfo) ExtInfo = new struct ExtInfo();
     return *ExtInfo;
   }
 
   const struct ExtInfo &getExtInfo() const {
     if (!ExtInfo) ExtInfo = new struct ExtInfo();
     return *ExtInfo;
public:
   size_t getAllocatedSize() const {
     return sizeof(EHCleanupScope) + CleanupBits.CleanupSize;
   }
 
   EHCleanupScope(bool isNormal, bool isEH, bool isActive,
                  unsigned cleanupSize, unsigned fixupDepth,
                  EHScopeStack::stable_iterator enclosingNormal,
                  EHScopeStack::stable_iterator enclosingEH)
-    : EHScope(EHScope::Cleanup, enclosingEH), EnclosingNormal(enclosingNormal),
-      NormalBlock(nullptr), ActiveFlag(nullptr), ExtInfo(nullptr) {
+      : EHScope(EHScope::Cleanup, enclosingEH),
+        EnclosingNormal(enclosingNormal), NormalBlock(nullptr),
+        ActiveFlag(nullptr), ExtInfo(nullptr), FixupDepth(fixupDepth) {
     CleanupBits.IsNormalCleanup = isNormal;
     CleanupBits.IsEHCleanup = isEH;
     CleanupBits.IsActive = isActive;
     CleanupBits.IsLifetimeMarker = false;
     CleanupBits.TestFlagInNormalCleanup = false;
     CleanupBits.TestFlagInEHCleanup = false;
     CleanupBits.CleanupSize = cleanupSize;
-    CleanupBits.FixupDepth = fixupDepth;
 
     assert(CleanupBits.CleanupSize == cleanupSize && "cleanup size overflow");
   }
 
   void Destroy() {
     delete ExtInfo;
   }
   // Objects of EHCleanupScope are not destructed. Use Destroy().
public:
 
   void setTestFlagInEHCleanup() {
     CleanupBits.TestFlagInEHCleanup = true;
   }
   bool shouldTestFlagInEHCleanup() const {
     return CleanupBits.TestFlagInEHCleanup;
   }
 
-  unsigned getFixupDepth() const { return CleanupBits.FixupDepth; }
+  unsigned getFixupDepth() const { return FixupDepth; }
   EHScopeStack::stable_iterator getEnclosingNormalCleanup() const {
     return EnclosingNormal;
   }
 
   size_t getCleanupSize() const { return CleanupBits.CleanupSize; }
   void *getCleanupBuffer() { return this + 1; }
 
   EHScopeStack::Cleanup *getCleanup() {
class EHFilterScope : public EHScope {
   llvm::Value * const *getFilters() const {
     return reinterpret_cast<llvm::Value* const *>(this+1);
   }
 
 public:
   EHFilterScope(unsigned numFilters)
     : EHScope(Filter, EHScopeStack::stable_end()) {
     FilterBits.NumFilters = numFilters;
+    assert(FilterBits.NumFilters == numFilters && "NumFilters overflow");
   }
 
   static size_t getSizeForNumFilters(unsigned numFilters) {
     return sizeof(EHFilterScope) + numFilters * sizeof(llvm::Value*);
   }
 
   unsigned getNumFilters() const { return FilterBits.NumFilters; }
 

cfe/trunk/test/CodeGen/fixup-depth-overflow.c

+// RUN: %clang_cc1 -O1 -disable-llvm-optzns -emit-llvm -o - %s | FileCheck %s
+
+#define M if (x) goto L1;
+#define M10 M M M M M M M M M M
+#define M100 M10 M10 M10 M10 M10 M10 M10 M10 M10 M10
+#define M1000 M100 M100 M100 M100 M100 M100 M100 M100 M100 M100
+
+void f(int x) {
+  int h;
+
+  // Many gotos to not-yet-emitted labels would cause EHScope's FixupDepth
+  // to overflow (PR23490).
+  M1000 M1000 M1000
+
+  if (x == 5) {
+    // This will cause us to emit a clean-up of the stack variable. If the
+    // FixupDepths are broken, fixups will erroneously get threaded through it.
+    int i;
+  }
+
+L1:
+  return;
+}
+
+// CHECK-LABEL: define void @f
+// CHECK-NOT: cleanup