[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-    // These don't need to be particularly wide, because they're
-    // strictly limited by the forms of expressions we permit.
-    unsigned NumSubExprs : 8;
-    unsigned ResultIndex : 32 - 8 - NumExprBits;
+    unsigned NumSubExprs : 16;
+    unsigned ResultIndex : 16;
   };
----------------
dblaikie wrote:
> aaron.ballman wrote:
> > dblaikie wrote:
> > > Could/should we add some error checking in the ctor to assert that we don't overflow these longer values/just hit the bug later on?
> > > 
> > > (& could we use `unsigned short` here rather than bitfields?)
> > We've already got them packed in with other bit-fields from the expression bits, so I think it's reasonable to continue the pattern of using bit-fields (that way we don't accidentally end up with padding between the unnamed bits at the start and the named bits in this object).
> > 
> > I think adding some assertions would not be a bad idea as a follow-up.
> Maybe some unconditional (rather than only in asserts builds) error handling? (report_fatal_error, if this is low priority enough to not have an elegant failure mode, but something where we don't just overflow and carry on would be good... )
Ping on this? I worry this code has just punted the same bug further down, but not plugged the hole/ensured we don't overflow on novel/larger inputs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154784/new/

https://reviews.llvm.org/D154784