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

[Yurong via Phabricator via cfe-commits]

yronglin marked 2 inline comments as done.
yronglin 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:
> > yronglin wrote:
> > > aaron.ballman wrote:
> > > > dblaikie wrote:
> > > > > aaron.ballman wrote:
> > > > > > yronglin wrote:
> > > > > > > yronglin wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > yronglin wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > yronglin wrote:
> > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > Sorry for the late reply, I was looking through the 
> > > > > > > > > > > > > emails and found this. I agree add some assertions to 
> > > > > > > > > > > > > check the value is a good idea, It's easy to help 
> > > > > > > > > > > > > people catch bugs, at least with when 
> > > > > > > > > > > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work 
> > > > > > > > > > > > > on it, but one thing that worries me is that, in 
> > > > > > > > > > > > > ASTReader, we access this field directly, not through 
> > > > > > > > > > > > > the constructor or accessor, and we have to add 
> > > > > > > > > > > > > assertions everywhere. 
> > > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > > > > > > > I don't think we have to add too many assertions. As 
> > > > > > > > > > > > best I can tell, we'll need one in each of the 
> > > > > > > > > > > > `PseudoObjectExpr` constructors and one in 
> > > > > > > > > > > > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are 
> > > > > > > > > > > > the only places we assign a value into the bit-field. 
> > > > > > > > > > > > Three assertions isn't a lot, but if we're worried, we 
> > > > > > > > > > > > could add a setter method that does the assertion and 
> > > > > > > > > > > > use the setter in all three places.
> > > > > > > > > > > My concern wasn't (well, wasn't entirely) about adding 
> > > > > > > > > > > more assertions - but about having a reliable error here. 
> > > > > > > > > > > The patch only makes the sizes larger, but doesn't have a 
> > > > > > > > > > > hard-stop in case those sizes are exceeded again (which, 
> > > > > > > > > > > admittedly, is much harder to do - maybe it's totally 
> > > > > > > > > > > unreachable now, for all practical purposes?) 
> > > > > > > > > > > 
> > > > > > > > > > > I suspect with more carefully constructed recursive 
> > > > > > > > > > > inputs could still reach the higher limit & I think it'd 
> > > > > > > > > > > be good to fail hard in that case in some way? (it's 
> > > > > > > > > > > probably rare enough that a report_fatal_error would 

[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;
   };

aaron.ballman wrote:
> yronglin wrote:
> > aaron.ballman wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > yronglin wrote:
> > > > > > yronglin wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > yronglin wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > yronglin wrote:
> > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > 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.
> > > > > > > > > > > > Sorry for the late reply, I was looking through the 
> > > > > > > > > > > > emails and found this. I agree add some assertions to 
> > > > > > > > > > > > check the value is a good idea, It's easy to help 
> > > > > > > > > > > > people catch bugs, at least with when 
> > > > > > > > > > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on 
> > > > > > > > > > > > it, but one thing that worries me is that, in 
> > > > > > > > > > > > ASTReader, we access this field directly, not through 
> > > > > > > > > > > > the constructor or accessor, and we have to add 
> > > > > > > > > > > > assertions everywhere. 
> > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > > > > > > I don't think we have to add too many assertions. As best 
> > > > > > > > > > > I can tell, we'll need one in each of the 
> > > > > > > > > > > `PseudoObjectExpr` constructors and one in 
> > > > > > > > > > > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are 
> > > > > > > > > > > the only places we assign a value into the bit-field. 
> > > > > > > > > > > Three assertions isn't a lot, but if we're worried, we 
> > > > > > > > > > > could add a setter method that does the assertion and use 
> > > > > > > > > > > the setter in all three places.
> > > > > > > > > > My concern wasn't (well, wasn't entirely) about adding more 
> > > > > > > > > > assertions - but about having a reliable error here. The 
> > > > > > > > > > patch only makes the sizes larger, but doesn't have a 
> > > > > > > > > > hard-stop in case those sizes are exceeded again (which, 
> > > > > > > > > > admittedly, is much harder to do - maybe it's totally 
> > > > > > > > > > unreachable now, for all practical purposes?) 
> > > > > > > > > > 
> > > > > > > > > > I suspect with more carefully constructed recursive inputs 
> > > > > > > > > > could still reach the higher limit & I think it'd be good 
> > > > > > > > > > to fail hard in that case in some way? (it's probably rare 
> > > > > > > > > > enough that a report_fatal_error would be 
> > > > > > > > > > not-the-worst-thing-ever)
> > > > > > > > > > 
> > > > > > > > > > But good assertions would be nice too (the old code only 
> > > > > > > > > > failed when you hit /exactly/ on just the overflow value, 
> > > > > > > > > > and any more 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
   };

yronglin wrote:
> aaron.ballman wrote:
> > dblaikie wrote:
> > > aaron.ballman wrote:
> > > > yronglin wrote:
> > > > > yronglin wrote:
> > > > > > dblaikie wrote:
> > > > > > > yronglin wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > yronglin wrote:
> > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > 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.
> > > > > > > > > > > Sorry for the late reply, I was looking through the 
> > > > > > > > > > > emails and found this. I agree add some assertions to 
> > > > > > > > > > > check the value is a good idea, It's easy to help people 
> > > > > > > > > > > catch bugs, at least with when 
> > > > > > > > > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on 
> > > > > > > > > > > it, but one thing that worries me is that, in ASTReader, 
> > > > > > > > > > > we access this field directly, not through the 
> > > > > > > > > > > constructor or accessor, and we have to add assertions 
> > > > > > > > > > > everywhere. 
> > > > > > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > > > > > I don't think we have to add too many assertions. As best I 
> > > > > > > > > > can tell, we'll need one in each of the `PseudoObjectExpr` 
> > > > > > > > > > constructors and one in 
> > > > > > > > > > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the 
> > > > > > > > > > only places we assign a value into the bit-field. Three 
> > > > > > > > > > assertions isn't a lot, but if we're worried, we could add 
> > > > > > > > > > a setter method that does the assertion and use the setter 
> > > > > > > > > > in all three places.
> > > > > > > > > My concern wasn't (well, wasn't entirely) about adding more 
> > > > > > > > > assertions - but about having a reliable error here. The 
> > > > > > > > > patch only makes the sizes larger, but doesn't have a 
> > > > > > > > > hard-stop in case those sizes are exceeded again (which, 
> > > > > > > > > admittedly, is much harder to do - maybe it's totally 
> > > > > > > > > unreachable now, for all practical purposes?) 
> > > > > > > > > 
> > > > > > > > > I suspect with more carefully constructed recursive inputs 
> > > > > > > > > could still reach the higher limit & I think it'd be good to 
> > > > > > > > > fail hard in that case in some way? (it's probably rare 
> > > > > > > > > enough that a report_fatal_error would be 
> > > > > > > > > not-the-worst-thing-ever)
> > > > > > > > > 
> > > > > > > > > But good assertions would be nice too (the old code only 
> > > > > > > > > failed when you hit /exactly/ on just the overflow value, and 
> > > > > > > > > any more than that the wraparound would not crash/fail, but 
> > > > > > > > > misbehave) - I did add the necessary assertion to ArrayRef 
> > > > > > > > > (begin <= end) which would've helped 

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

[Yurong via Phabricator via cfe-commits]

yronglin marked 2 inline comments as done.
yronglin 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;
   };

aaron.ballman wrote:
> dblaikie wrote:
> > aaron.ballman wrote:
> > > yronglin wrote:
> > > > yronglin wrote:
> > > > > dblaikie wrote:
> > > > > > yronglin wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > yronglin wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > 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.
> > > > > > > > > > Sorry for the late reply, I was looking through the emails 
> > > > > > > > > > and found this. I agree add some assertions to check the 
> > > > > > > > > > value is a good idea, It's easy to help people catch bugs, 
> > > > > > > > > > at least with when `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm 
> > > > > > > > > > glad to work on it, but one thing that worries me is that, 
> > > > > > > > > > in ASTReader, we access this field directly, not through 
> > > > > > > > > > the constructor or accessor, and we have to add assertions 
> > > > > > > > > > everywhere. 
> > > > > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > > > > I don't think we have to add too many assertions. As best I 
> > > > > > > > > can tell, we'll need one in each of the `PseudoObjectExpr` 
> > > > > > > > > constructors and one in 
> > > > > > > > > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the 
> > > > > > > > > only places we assign a value into the bit-field. Three 
> > > > > > > > > assertions isn't a lot, but if we're worried, we could add a 
> > > > > > > > > setter method that does the assertion and use the setter in 
> > > > > > > > > all three places.
> > > > > > > > My concern wasn't (well, wasn't entirely) about adding more 
> > > > > > > > assertions - but about having a reliable error here. The patch 
> > > > > > > > only makes the sizes larger, but doesn't have a hard-stop in 
> > > > > > > > case those sizes are exceeded again (which, admittedly, is much 
> > > > > > > > harder to do - maybe it's totally unreachable now, for all 
> > > > > > > > practical purposes?) 
> > > > > > > > 
> > > > > > > > I suspect with more carefully constructed recursive inputs 
> > > > > > > > could still reach the higher limit & I think it'd be good to 
> > > > > > > > fail hard in that case in some way? (it's probably rare enough 
> > > > > > > > that a report_fatal_error would be not-the-worst-thing-ever)
> > > > > > > > 
> > > > > > > > But good assertions would be nice too (the old code only failed 
> > > > > > > > when you hit /exactly/ on just the overflow value, and any more 
> > > > > > > > than that the wraparound would not crash/fail, but misbehave) - 
> > > > > > > > I did add the necessary assertion to ArrayRef (begin <= end) 
> > > > > > > > which would've helped detect this more reliably, but some 
> > > > > > > > assert checking for overflow in the ctor would be good too 
> > > > > > > > (with all the usual nuance/care in 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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:
> > yronglin wrote:
> > > yronglin wrote:
> > > > dblaikie wrote:
> > > > > yronglin wrote:
> > > > > > dblaikie wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > yronglin wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > 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.
> > > > > > > > > Sorry for the late reply, I was looking through the emails 
> > > > > > > > > and found this. I agree add some assertions to check the 
> > > > > > > > > value is a good idea, It's easy to help people catch bugs, at 
> > > > > > > > > least with when `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad 
> > > > > > > > > to work on it, but one thing that worries me is that, in 
> > > > > > > > > ASTReader, we access this field directly, not through the 
> > > > > > > > > constructor or accessor, and we have to add assertions 
> > > > > > > > > everywhere. 
> > > > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > > > I don't think we have to add too many assertions. As best I can 
> > > > > > > > tell, we'll need one in each of the `PseudoObjectExpr` 
> > > > > > > > constructors and one in 
> > > > > > > > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the 
> > > > > > > > only places we assign a value into the bit-field. Three 
> > > > > > > > assertions isn't a lot, but if we're worried, we could add a 
> > > > > > > > setter method that does the assertion and use the setter in all 
> > > > > > > > three places.
> > > > > > > My concern wasn't (well, wasn't entirely) about adding more 
> > > > > > > assertions - but about having a reliable error here. The patch 
> > > > > > > only makes the sizes larger, but doesn't have a hard-stop in case 
> > > > > > > those sizes are exceeded again (which, admittedly, is much harder 
> > > > > > > to do - maybe it's totally unreachable now, for all practical 
> > > > > > > purposes?) 
> > > > > > > 
> > > > > > > I suspect with more carefully constructed recursive inputs could 
> > > > > > > still reach the higher limit & I think it'd be good to fail hard 
> > > > > > > in that case in some way? (it's probably rare enough that a 
> > > > > > > report_fatal_error would be not-the-worst-thing-ever)
> > > > > > > 
> > > > > > > But good assertions would be nice too (the old code only failed 
> > > > > > > when you hit /exactly/ on just the overflow value, and any more 
> > > > > > > than that the wraparound would not crash/fail, but misbehave) - I 
> > > > > > > did add the necessary assertion to ArrayRef (begin <= end) which 
> > > > > > > would've helped detect this more reliably, but some assert 
> > > > > > > checking for overflow in the ctor would be good too (with all the 
> > > > > > > usual nuance/care in checking for overflow) - unless we're going 
> > > > > > > to make that into a fatal or other real error.
> > > > > > Sorry for the very late reply. I have no preference between 
> > > > > > assertion and 

[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;
   };

aaron.ballman wrote:
> yronglin wrote:
> > yronglin wrote:
> > > dblaikie wrote:
> > > > yronglin wrote:
> > > > > dblaikie wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > yronglin wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > 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.
> > > > > > > > Sorry for the late reply, I was looking through the emails and 
> > > > > > > > found this. I agree add some assertions to check the value is a 
> > > > > > > > good idea, It's easy to help people catch bugs, at least with 
> > > > > > > > when `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, 
> > > > > > > > but one thing that worries me is that, in ASTReader, we access 
> > > > > > > > this field directly, not through the constructor or accessor, 
> > > > > > > > and we have to add assertions everywhere. 
> > > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > > I don't think we have to add too many assertions. As best I can 
> > > > > > > tell, we'll need one in each of the `PseudoObjectExpr` 
> > > > > > > constructors and one in `ASTStmtReader::VisitPseudoObjectExpr()`, 
> > > > > > > but those are the only places we assign a value into the 
> > > > > > > bit-field. Three assertions isn't a lot, but if we're worried, we 
> > > > > > > could add a setter method that does the assertion and use the 
> > > > > > > setter in all three places.
> > > > > > My concern wasn't (well, wasn't entirely) about adding more 
> > > > > > assertions - but about having a reliable error here. The patch only 
> > > > > > makes the sizes larger, but doesn't have a hard-stop in case those 
> > > > > > sizes are exceeded again (which, admittedly, is much harder to do - 
> > > > > > maybe it's totally unreachable now, for all practical purposes?) 
> > > > > > 
> > > > > > I suspect with more carefully constructed recursive inputs could 
> > > > > > still reach the higher limit & I think it'd be good to fail hard in 
> > > > > > that case in some way? (it's probably rare enough that a 
> > > > > > report_fatal_error would be not-the-worst-thing-ever)
> > > > > > 
> > > > > > But good assertions would be nice too (the old code only failed 
> > > > > > when you hit /exactly/ on just the overflow value, and any more 
> > > > > > than that the wraparound would not crash/fail, but misbehave) - I 
> > > > > > did add the necessary assertion to ArrayRef (begin <= end) which 
> > > > > > would've helped detect this more reliably, but some assert checking 
> > > > > > for overflow in the ctor would be good too (with all the usual 
> > > > > > nuance/care in checking for overflow) - unless we're going to make 
> > > > > > that into a fatal or other real error.
> > > > > Sorry for the very late reply. I have no preference between assertion 
> > > > > and `llvm_unreachable`, if error then fail fast is looks good. I have 
> > > > > a patch D158296 to add assertion.
> > > > Thanks for the assertions - though they still haven't met my main 
> > > > concern that this 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
   };

yronglin wrote:
> yronglin wrote:
> > dblaikie wrote:
> > > yronglin wrote:
> > > > dblaikie wrote:
> > > > > aaron.ballman wrote:
> > > > > > yronglin wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > 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.
> > > > > > > Sorry for the late reply, I was looking through the emails and 
> > > > > > > found this. I agree add some assertions to check the value is a 
> > > > > > > good idea, It's easy to help people catch bugs, at least with 
> > > > > > > when `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, 
> > > > > > > but one thing that worries me is that, in ASTReader, we access 
> > > > > > > this field directly, not through the constructor or accessor, and 
> > > > > > > we have to add assertions everywhere. 
> > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > I don't think we have to add too many assertions. As best I can 
> > > > > > tell, we'll need one in each of the `PseudoObjectExpr` constructors 
> > > > > > and one in `ASTStmtReader::VisitPseudoObjectExpr()`, but those are 
> > > > > > the only places we assign a value into the bit-field. Three 
> > > > > > assertions isn't a lot, but if we're worried, we could add a setter 
> > > > > > method that does the assertion and use the setter in all three 
> > > > > > places.
> > > > > My concern wasn't (well, wasn't entirely) about adding more 
> > > > > assertions - but about having a reliable error here. The patch only 
> > > > > makes the sizes larger, but doesn't have a hard-stop in case those 
> > > > > sizes are exceeded again (which, admittedly, is much harder to do - 
> > > > > maybe it's totally unreachable now, for all practical purposes?) 
> > > > > 
> > > > > I suspect with more carefully constructed recursive inputs could 
> > > > > still reach the higher limit & I think it'd be good to fail hard in 
> > > > > that case in some way? (it's probably rare enough that a 
> > > > > report_fatal_error would be not-the-worst-thing-ever)
> > > > > 
> > > > > But good assertions would be nice too (the old code only failed when 
> > > > > you hit /exactly/ on just the overflow value, and any more than that 
> > > > > the wraparound would not crash/fail, but misbehave) - I did add the 
> > > > > necessary assertion to ArrayRef (begin <= end) which would've helped 
> > > > > detect this more reliably, but some assert checking for overflow in 
> > > > > the ctor would be good too (with all the usual nuance/care in 
> > > > > checking for overflow) - unless we're going to make that into a fatal 
> > > > > or other real error.
> > > > Sorry for the very late reply. I have no preference between assertion 
> > > > and `llvm_unreachable`, if error then fail fast is looks good. I have a 
> > > > patch D158296 to add assertion.
> > > Thanks for the assertions - though they still haven't met my main concern 
> > > that this should have a hard failure even in a non-assertions build.
> > > 
> > > I know we don't have a perfect plan/policy for these sort of "run out of 
> > > resources/hit a representational limit" issues (at 

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

[Yurong via Phabricator via cfe-commits]

yronglin 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;
   };

yronglin wrote:
> dblaikie wrote:
> > yronglin wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > yronglin wrote:
> > > > > > dblaikie wrote:
> > > > > > > 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.
> > > > > > Sorry for the late reply, I was looking through the emails and 
> > > > > > found this. I agree add some assertions to check the value is a 
> > > > > > good idea, It's easy to help people catch bugs, at least with when 
> > > > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, but one 
> > > > > > thing that worries me is that, in ASTReader, we access this field 
> > > > > > directly, not through the constructor or accessor, and we have to 
> > > > > > add assertions everywhere. 
> > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > I don't think we have to add too many assertions. As best I can tell, 
> > > > > we'll need one in each of the `PseudoObjectExpr` constructors and one 
> > > > > in `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only 
> > > > > places we assign a value into the bit-field. Three assertions isn't a 
> > > > > lot, but if we're worried, we could add a setter method that does the 
> > > > > assertion and use the setter in all three places.
> > > > My concern wasn't (well, wasn't entirely) about adding more assertions 
> > > > - but about having a reliable error here. The patch only makes the 
> > > > sizes larger, but doesn't have a hard-stop in case those sizes are 
> > > > exceeded again (which, admittedly, is much harder to do - maybe it's 
> > > > totally unreachable now, for all practical purposes?) 
> > > > 
> > > > I suspect with more carefully constructed recursive inputs could still 
> > > > reach the higher limit & I think it'd be good to fail hard in that case 
> > > > in some way? (it's probably rare enough that a report_fatal_error would 
> > > > be not-the-worst-thing-ever)
> > > > 
> > > > But good assertions would be nice too (the old code only failed when 
> > > > you hit /exactly/ on just the overflow value, and any more than that 
> > > > the wraparound would not crash/fail, but misbehave) - I did add the 
> > > > necessary assertion to ArrayRef (begin <= end) which would've helped 
> > > > detect this more reliably, but some assert checking for overflow in the 
> > > > ctor would be good too (with all the usual nuance/care in checking for 
> > > > overflow) - unless we're going to make that into a fatal or other real 
> > > > error.
> > > Sorry for the very late reply. I have no preference between assertion and 
> > > `llvm_unreachable`, if error then fail fast is looks good. I have a patch 
> > > D158296 to add assertion.
> > Thanks for the assertions - though they still haven't met my main concern 
> > that this should have a hard failure even in a non-assertions build.
> > 
> > I know we don't have a perfect plan/policy for these sort of "run out of 
> > resources/hit a representational limit" issues (at least I don't think we 
> > do... do we, @aaron.ballman ? I know we have some limits (recursion, 
> > template expansion, etc) but they're fairly specific/aren't about every 
> > 

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

[Yurong via Phabricator via cfe-commits]

yronglin marked an inline comment as done.
yronglin 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:
> yronglin wrote:
> > dblaikie wrote:
> > > aaron.ballman wrote:
> > > > yronglin wrote:
> > > > > dblaikie wrote:
> > > > > > 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.
> > > > > Sorry for the late reply, I was looking through the emails and found 
> > > > > this. I agree add some assertions to check the value is a good idea, 
> > > > > It's easy to help people catch bugs, at least with when 
> > > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, but one 
> > > > > thing that worries me is that, in ASTReader, we access this field 
> > > > > directly, not through the constructor or accessor, and we have to add 
> > > > > assertions everywhere. 
> > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > I don't think we have to add too many assertions. As best I can tell, 
> > > > we'll need one in each of the `PseudoObjectExpr` constructors and one 
> > > > in `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only 
> > > > places we assign a value into the bit-field. Three assertions isn't a 
> > > > lot, but if we're worried, we could add a setter method that does the 
> > > > assertion and use the setter in all three places.
> > > My concern wasn't (well, wasn't entirely) about adding more assertions - 
> > > but about having a reliable error here. The patch only makes the sizes 
> > > larger, but doesn't have a hard-stop in case those sizes are exceeded 
> > > again (which, admittedly, is much harder to do - maybe it's totally 
> > > unreachable now, for all practical purposes?) 
> > > 
> > > I suspect with more carefully constructed recursive inputs could still 
> > > reach the higher limit & I think it'd be good to fail hard in that case 
> > > in some way? (it's probably rare enough that a report_fatal_error would 
> > > be not-the-worst-thing-ever)
> > > 
> > > But good assertions would be nice too (the old code only failed when you 
> > > hit /exactly/ on just the overflow value, and any more than that the 
> > > wraparound would not crash/fail, but misbehave) - I did add the necessary 
> > > assertion to ArrayRef (begin <= end) which would've helped detect this 
> > > more reliably, but some assert checking for overflow in the ctor would be 
> > > good too (with all the usual nuance/care in checking for overflow) - 
> > > unless we're going to make that into a fatal or other real error.
> > Sorry for the very late reply. I have no preference between assertion and 
> > `llvm_unreachable`, if error then fail fast is looks good. I have a patch 
> > D158296 to add assertion.
> Thanks for the assertions - though they still haven't met my main concern 
> that this should have a hard failure even in a non-assertions build.
> 
> I know we don't have a perfect plan/policy for these sort of "run out of 
> resources/hit a representational limit" issues (at least I don't think we 
> do... do we, @aaron.ballman ? I know we have some limits (recursion, template 
> expansion, etc) but they're fairly specific/aren't about every possible case 
> of integer overflow in some representational element, etc) but we've seen 
> this one is pretty reachable. 
> 
> Here's a test case 

[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;
   };

yronglin wrote:
> dblaikie wrote:
> > aaron.ballman wrote:
> > > yronglin wrote:
> > > > dblaikie wrote:
> > > > > 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.
> > > > Sorry for the late reply, I was looking through the emails and found 
> > > > this. I agree add some assertions to check the value is a good idea, 
> > > > It's easy to help people catch bugs, at least with when 
> > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, but one 
> > > > thing that worries me is that, in ASTReader, we access this field 
> > > > directly, not through the constructor or accessor, and we have to add 
> > > > assertions everywhere. 
> > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > I don't think we have to add too many assertions. As best I can tell, 
> > > we'll need one in each of the `PseudoObjectExpr` constructors and one in 
> > > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only places 
> > > we assign a value into the bit-field. Three assertions isn't a lot, but 
> > > if we're worried, we could add a setter method that does the assertion 
> > > and use the setter in all three places.
> > My concern wasn't (well, wasn't entirely) about adding more assertions - 
> > but about having a reliable error here. The patch only makes the sizes 
> > larger, but doesn't have a hard-stop in case those sizes are exceeded again 
> > (which, admittedly, is much harder to do - maybe it's totally unreachable 
> > now, for all practical purposes?) 
> > 
> > I suspect with more carefully constructed recursive inputs could still 
> > reach the higher limit & I think it'd be good to fail hard in that case in 
> > some way? (it's probably rare enough that a report_fatal_error would be 
> > not-the-worst-thing-ever)
> > 
> > But good assertions would be nice too (the old code only failed when you 
> > hit /exactly/ on just the overflow value, and any more than that the 
> > wraparound would not crash/fail, but misbehave) - I did add the necessary 
> > assertion to ArrayRef (begin <= end) which would've helped detect this more 
> > reliably, but some assert checking for overflow in the ctor would be good 
> > too (with all the usual nuance/care in checking for overflow) - unless 
> > we're going to make that into a fatal or other real error.
> Sorry for the very late reply. I have no preference between assertion and 
> `llvm_unreachable`, if error then fail fast is looks good. I have a patch 
> D158296 to add assertion.
Thanks for the assertions - though they still haven't met my main concern that 
this should have a hard failure even in a non-assertions build.

I know we don't have a perfect plan/policy for these sort of "run out of 
resources/hit a representational limit" issues (at least I don't think we do... 
do we, @aaron.ballman ? I know we have some limits (recursion, template 
expansion, etc) but they're fairly specific/aren't about every possible case of 
integer overflow in some representational element, etc) but we've seen this one 
is pretty reachable. 

Here's a test case that would still trigger the assertion, and trigger UB in a 
non-assertions build:
```
truct t1 { };
template
struct templ {
T1 v1;
T1 v2;
T1 v3;
T1 v4;
};

struct t2 {
  templ> c0;
  

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

[Yurong via Phabricator via cfe-commits]

yronglin marked 3 inline comments as done.
yronglin 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:
> > yronglin wrote:
> > > dblaikie wrote:
> > > > 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.
> > > Sorry for the late reply, I was looking through the emails and found 
> > > this. I agree add some assertions to check the value is a good idea, It's 
> > > easy to help people catch bugs, at least with when 
> > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, but one thing 
> > > that worries me is that, in ASTReader, we access this field directly, not 
> > > through the constructor or accessor, and we have to add assertions 
> > > everywhere. 
> > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > I don't think we have to add too many assertions. As best I can tell, we'll 
> > need one in each of the `PseudoObjectExpr` constructors and one in 
> > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only places we 
> > assign a value into the bit-field. Three assertions isn't a lot, but if 
> > we're worried, we could add a setter method that does the assertion and use 
> > the setter in all three places.
> My concern wasn't (well, wasn't entirely) about adding more assertions - but 
> about having a reliable error here. The patch only makes the sizes larger, 
> but doesn't have a hard-stop in case those sizes are exceeded again (which, 
> admittedly, is much harder to do - maybe it's totally unreachable now, for 
> all practical purposes?) 
> 
> I suspect with more carefully constructed recursive inputs could still reach 
> the higher limit & I think it'd be good to fail hard in that case in some 
> way? (it's probably rare enough that a report_fatal_error would be 
> not-the-worst-thing-ever)
> 
> But good assertions would be nice too (the old code only failed when you hit 
> /exactly/ on just the overflow value, and any more than that the wraparound 
> would not crash/fail, but misbehave) - I did add the necessary assertion to 
> ArrayRef (begin <= end) which would've helped detect this more reliably, but 
> some assert checking for overflow in the ctor would be good too (with all the 
> usual nuance/care in checking for overflow) - unless we're going to make that 
> into a fatal or other real error.
Sorry for the very late reply. I have no preference between assertion and 
`llvm_unreachable`, if error then fail fast is looks good. I have a patch 
D158296 to add assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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;
   };

aaron.ballman wrote:
> yronglin wrote:
> > dblaikie wrote:
> > > 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.
> > Sorry for the late reply, I was looking through the emails and found this. 
> > I agree add some assertions to check the value is a good idea, It's easy to 
> > help people catch bugs, at least with when `-DLLVM_ENABLE_ASSERTIONS=ON`, 
> > and I'm glad to work on it, but one thing that worries me is that, in 
> > ASTReader, we access this field directly, not through the constructor or 
> > accessor, and we have to add assertions everywhere. 
> > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> I don't think we have to add too many assertions. As best I can tell, we'll 
> need one in each of the `PseudoObjectExpr` constructors and one in 
> `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only places we 
> assign a value into the bit-field. Three assertions isn't a lot, but if we're 
> worried, we could add a setter method that does the assertion and use the 
> setter in all three places.
My concern wasn't (well, wasn't entirely) about adding more assertions - but 
about having a reliable error here. The patch only makes the sizes larger, but 
doesn't have a hard-stop in case those sizes are exceeded again (which, 
admittedly, is much harder to do - maybe it's totally unreachable now, for all 
practical purposes?) 

I suspect with more carefully constructed recursive inputs could still reach 
the higher limit & I think it'd be good to fail hard in that case in some way? 
(it's probably rare enough that a report_fatal_error would be 
not-the-worst-thing-ever)

But good assertions would be nice too (the old code only failed when you hit 
/exactly/ on just the overflow value, and any more than that the wraparound 
would not crash/fail, but misbehave) - I did add the necessary assertion to 
ArrayRef (begin <= end) which would've helped detect this more reliably, but 
some assert checking for overflow in the ctor would be good too (with all the 
usual nuance/care in checking for overflow) - unless we're going to make that 
into a fatal or other real error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
   };

yronglin wrote:
> dblaikie wrote:
> > 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.
> Sorry for the late reply, I was looking through the emails and found this. I 
> agree add some assertions to check the value is a good idea, It's easy to 
> help people catch bugs, at least with when `-DLLVM_ENABLE_ASSERTIONS=ON`, and 
> I'm glad to work on it, but one thing that worries me is that, in ASTReader, 
> we access this field directly, not through the constructor or accessor, and 
> we have to add assertions everywhere. 
> https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
I don't think we have to add too many assertions. As best I can tell, we'll 
need one in each of the `PseudoObjectExpr` constructors and one in 
`ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only places we 
assign a value into the bit-field. Three assertions isn't a lot, but if we're 
worried, we could add a setter method that does the assertion and use the 
setter in all three places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin marked 4 inline comments as done.
yronglin 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:
> 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.
Sorry for the late reply, I was looking through the emails and found this. I 
agree add some assertions to check the value is a good idea, It's easy to help 
people catch bugs, at least with when `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm 
glad to work on it, but one thing that worries me is that, in ASTReader, we 
access this field directly, not through the constructor or accessor, and we 
have to add assertions everywhere. 
https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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;
   };

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... )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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;
   };

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?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG677a1da6fafd: [clang] Fix crash caused by 
PseudoObjectExprBitfields::NumSubExprs overflow (authored by yronglin).

Changed prior to commit:
  https://reviews.llvm.org/D154784?vs=539563=539798#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Stmt.h
  clang/test/SemaCXX/builtin-dump-struct.cpp


Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known 
conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, 
v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, 
v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, 
v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, 
v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, 
v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, 
v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, 
v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, 
v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, 
v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, 
v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, 
v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -593,10 +593,8 @@
 
 unsigned : NumExprBits;
 
-// 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;
   };
 
   class SourceLocExprBitfields {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -587,6 +587,8 @@
   (`#50320 `_).
 - Fix an assertion when using ``\u0024`` (``$``) as an identifier, by 
disallowing
   that construct (`#62133 
_`).
+- Fix crash caused by PseudoObjectExprBitfields: NumSubExprs overflow.
+  (`#63169 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  

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

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

Wait for CI green


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 539563.
yronglin added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Stmt.h
  clang/test/SemaCXX/builtin-dump-struct.cpp


Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known 
conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, 
v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, 
v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, 
v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, 
v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, 
v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, 
v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, 
v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, 
v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, 
v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, 
v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, 
v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -593,10 +593,8 @@
 
 unsigned : NumExprBits;
 
-// 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;
   };
 
   class SourceLocExprBitfields {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -591,6 +591,8 @@
   (`#38717 _`).
 - Fix an assertion when using ``\u0024`` (``$``) as an identifier, by 
disallowing
   that construct (`#62133 
_`).
+- Fix crash caused by PseudoObjectExprBitfields: NumSubExprs overflow.
+  (`#63169 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/include/clang/AST/Stmt.h

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

[Yurong via Phabricator via cfe-commits]

yronglin marked an inline comment as done.
yronglin added a comment.

Thanks for your review! @aaron.ballman @rjmccall


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin marked an inline comment as done.
yronglin added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:603
 
-// 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;
+// Whether the PseudoObjectExpr has result.
+unsigned HasResult : 1;

rjmccall wrote:
> aaron.ballman wrote:
> > 
> Please remove the comment, which is incorrect.  Otherwise, I think this is 
> fine.
Thanks, removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 539493.
yronglin added a comment.

Address comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Stmt.h
  clang/test/SemaCXX/builtin-dump-struct.cpp


Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known 
conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, 
v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, 
v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, 
v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, 
v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, 
v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, 
v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, 
v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, 
v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, 
v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, 
v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, 
v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -593,10 +593,8 @@
 
 unsigned : NumExprBits;
 
-// 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;
   };
 
   class SourceLocExprBitfields {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -591,6 +591,8 @@
   (`#38717 _`).
 - Fix an assertion when using ``\u0024`` (``$``) as an identifier, by 
disallowing
   that construct (`#62133 
_`).
+- Fix crash caused by PseudoObjectExprBitfields: NumSubExprs overflow.
+  (`#63169 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/include/clang/AST/Stmt.h

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you for the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:603
 
-// 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;
+// Whether the PseudoObjectExpr has result.
+unsigned HasResult : 1;

aaron.ballman wrote:
> 
Please remove the comment, which is incorrect.  Otherwise, I think this is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 539152.
yronglin added a comment.

Address John's comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Stmt.h
  clang/test/SemaCXX/builtin-dump-struct.cpp


Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known 
conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, 
v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, 
v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, 
v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, 
v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, 
v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, 
v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, 
v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, 
v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, 
v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, 
v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, 
v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -595,8 +595,8 @@
 
 // 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;
   };
 
   class SourceLocExprBitfields {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -582,6 +582,8 @@
   (`#50243 `_),
   (`#48636 `_),
   (`#50320 `_).
+- Fix crash caused by PseudoObjectExprBitfields: NumSubExprs overflow.
+  (`#63169 _`)
 
 Bug Fixes to Compiler Builtins
 ^^


Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/include/clang/AST/Stmt.h
===
--- 

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

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

In D154784#4486721 , @rjmccall wrote:

> We had that discussion in the bug report.  Let's just increase the widths 
> unconditionally; this is not such a common expression class that we need to 
> worry about using an extra word.

Thanks for your review!

> Let's just increase the widths unconditionally

Do you mean:

  class PseudoObjectExprBitfields {
  friend class ASTStmtReader; // deserialization
  friend class PseudoObjectExpr;
  
  unsigned : NumExprBits;
  unsigned NumSubExprs : 16;
  unsigned ResultIndex : 16;
};


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We had that discussion in the bug report.  Let's just increase the widths 
unconditionally; this is not such a common expression class that we need to 
worry about using an extra word.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

In D154784#4485752 , @aaron.ballman 
wrote:

> In general, I think this is a good approach. However, it sort of kicks the 
> can down the road a bit; we will still overflow the member if there are 
> enough fields. Would it make sense to also add a diagnostic to Sema so that 
> overflow with the widened fields is diagnosed rather than causing a crash?

Thanks you for take a look!

> Would it make sense to also add a diagnostic to Sema so that overflow with 
> the widened fields is diagnosed rather than causing a crash?

Ah, I think your are right. It doesn't make sense, As the comments for 
`PseudoObjectExprBitfields` says `These don't need to be particularly wide, 
because they're strictly limited by the forms of expressions we permit.` I 
think developers who use `PseudoObjectExprBitfields` need to be more careful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 538712.
yronglin marked 2 inline comments as done.
yronglin added a comment.

Update comments in PseudoObjectExpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/lib/AST/Expr.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/test/SemaCXX/builtin-dump-struct.cpp

Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1378,8 +1378,15 @@
 void ASTStmtReader::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
   VisitExpr(E);
   unsigned numSemanticExprs = Record.readInt();
-  assert(numSemanticExprs + 1 == E->PseudoObjectExprBits.NumSubExprs);
-  E->PseudoObjectExprBits.ResultIndex = Record.readInt();
+  assert(numSemanticExprs + 1 == E->getNumSubExprs());
+
+  unsigned ResultIndex = Record.readInt();
+  if (ResultIndex) {
+E->PseudoObjectExprBits.HasResult = true;
+E->PseudoObjectExprBits.ResultBits.ResultIndex = ResultIndex;
+  } else {
+E->PseudoObjectExprBits.HasResult = false;
+  }
 
   // Read the syntactic expression.
   E->getSubExprsBuffer()[0] = Record.readSubExpr();
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -4842,7 +4842,8 @@
 
 PseudoObjectExpr::PseudoObjectExpr(EmptyShell shell, unsigned numSemanticExprs)
   : Expr(PseudoObjectExprClass, shell) {
-  PseudoObjectExprBits.NumSubExprs = numSemanticExprs + 1;
+  PseudoObjectExprBits.HasResult = false;
+  PseudoObjectExprBits.NumSubExprsIfNoResult = numSemanticExprs + 1;
 }
 
 PseudoObjectExpr *PseudoObjectExpr::Create(const ASTContext , Expr *syntax,
@@ -4873,8 +4874,15 @@
Expr *syntax, ArrayRef semantics,
unsigned resultIndex)
 : Expr(PseudoObjectExprClass, type, VK, OK_Ordinary) {
-  PseudoObjectExprBits.NumSubExprs = semantics.size() + 1;
-  PseudoObjectExprBits.ResultIndex = resultIndex + 1;
+
+  if (resultIndex == NoResult) {
+PseudoObjectExprBits.HasResult = false;
+PseudoObjectExprBits.NumSubExprsIfNoResult = semantics.size() + 1;
+  } else {
+PseudoObjectExprBits.HasResult = true;
+PseudoObjectExprBits.ResultBits.NumSubExprs = semantics.size() + 1;
+PseudoObjectExprBits.ResultBits.ResultIndex = resultIndex + 1;
+  }
 
   for (unsigned i = 0, e = semantics.size() + 1; i != e; ++i) {
 Expr *E = (i == 0 ? syntax : semantics[i-1]);
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -591,12 +591,26 @@
 friend class ASTStmtReader; // deserialization
 friend class PseudoObjectExpr;
 
+struct ResultBitfields {
+  // These don't need to be particularly wide, because they're
+  // strictly limited by the forms of expressions we permit.
+  unsigned NumSubExprs : 16;
+  unsigned ResultIndex : 16;
+};
+
 unsigned 

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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 538709.
yronglin added a comment.

Address Aaron's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/lib/AST/Expr.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/test/SemaCXX/builtin-dump-struct.cpp

Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,28 @@
 // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1378,8 +1378,15 @@
 void ASTStmtReader::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
   VisitExpr(E);
   unsigned numSemanticExprs = Record.readInt();
-  assert(numSemanticExprs + 1 == E->PseudoObjectExprBits.NumSubExprs);
-  E->PseudoObjectExprBits.ResultIndex = Record.readInt();
+  assert(numSemanticExprs + 1 == E->getNumSubExprs());
+
+  unsigned ResultIndex = Record.readInt();
+  if (ResultIndex) {
+E->PseudoObjectExprBits.HasResult = true;
+E->PseudoObjectExprBits.ResultBits.ResultIndex = ResultIndex;
+  } else {
+E->PseudoObjectExprBits.HasResult = false;
+  }
 
   // Read the syntactic expression.
   E->getSubExprsBuffer()[0] = Record.readSubExpr();
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -4842,7 +4842,8 @@
 
 PseudoObjectExpr::PseudoObjectExpr(EmptyShell shell, unsigned numSemanticExprs)
   : Expr(PseudoObjectExprClass, shell) {
-  PseudoObjectExprBits.NumSubExprs = numSemanticExprs + 1;
+  PseudoObjectExprBits.HasResult = false;
+  PseudoObjectExprBits.NumSubExprsIfNoResult = numSemanticExprs + 1;
 }
 
 PseudoObjectExpr *PseudoObjectExpr::Create(const ASTContext , Expr *syntax,
@@ -4873,8 +4874,15 @@
Expr *syntax, ArrayRef semantics,
unsigned resultIndex)
 : Expr(PseudoObjectExprClass, type, VK, OK_Ordinary) {
-  PseudoObjectExprBits.NumSubExprs = semantics.size() + 1;
-  PseudoObjectExprBits.ResultIndex = resultIndex + 1;
+
+  if (resultIndex == NoResult) {
+PseudoObjectExprBits.HasResult = false;
+PseudoObjectExprBits.NumSubExprsIfNoResult = semantics.size() + 1;
+  } else {
+PseudoObjectExprBits.HasResult = true;
+PseudoObjectExprBits.ResultBits.NumSubExprs = semantics.size() + 1;
+PseudoObjectExprBits.ResultBits.ResultIndex = resultIndex + 1;
+  }
 
   for (unsigned i = 0, e = semantics.size() + 1; i != e; ++i) {
 Expr *E = (i == 0 ? syntax : semantics[i-1]);
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -591,12 +591,26 @@
 friend class ASTStmtReader; // deserialization
 friend class PseudoObjectExpr;
 
+struct ResultBitfields {
+  // These don't need to be particularly wide, because they're
+  // strictly limited by the forms of expressions we permit.
+  unsigned NumSubExprs : 16;
+  unsigned ResultIndex : 16;
+};
+
 unsigned : NumExprBits;
 
-// These don't need to be 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In general, I think this is a good approach. However, it sort of kicks the can 
down the road a bit; we will still overflow the member if there are enough 
fields. Would it make sense to also add a diagnostic to Sema so that overflow 
with the widened fields is diagnosed rather than causing a crash?




Comment at: clang/include/clang/AST/Stmt.h:603
 
-// 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;
+// Whether the PseudoObjectExpr has result.
+unsigned HasResult : 1;





Comment at: clang/test/SemaCXX/builtin-dump-struct.cpp:163
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow.
+struct t1 {




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin added a comment.

I'm not sure if we should limit the value of `NumSubExprs` when build  
`PseudoObjectExpr` for `__builtin_dump_struct`, This cost too much memory when 
the nested members of the record are very deep and the num of member is very 
large.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yurong via Phabricator via cfe-commits]

yronglin updated this revision to Diff 538418.
yronglin added a comment.

Format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/lib/AST/Expr.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/test/SemaCXX/builtin-dump-struct.cpp

Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,27 @@
 // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1378,8 +1378,15 @@
 void ASTStmtReader::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
   VisitExpr(E);
   unsigned numSemanticExprs = Record.readInt();
-  assert(numSemanticExprs + 1 == E->PseudoObjectExprBits.NumSubExprs);
-  E->PseudoObjectExprBits.ResultIndex = Record.readInt();
+  assert(numSemanticExprs + 1 == E->getNumSubExprs());
+
+  unsigned ResultIndex = Record.readInt();
+  if (ResultIndex) {
+E->PseudoObjectExprBits.HasResult = true;
+E->PseudoObjectExprBits.ResultBits.ResultIndex = ResultIndex;
+  } else {
+E->PseudoObjectExprBits.HasResult = false;
+  }
 
   // Read the syntactic expression.
   E->getSubExprsBuffer()[0] = Record.readSubExpr();
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -4848,7 +4848,8 @@
 
 PseudoObjectExpr::PseudoObjectExpr(EmptyShell shell, unsigned numSemanticExprs)
   : Expr(PseudoObjectExprClass, shell) {
-  PseudoObjectExprBits.NumSubExprs = numSemanticExprs + 1;
+  PseudoObjectExprBits.HasResult = false;
+  PseudoObjectExprBits.NumSubExprsIfNoResult = numSemanticExprs + 1;
 }
 
 PseudoObjectExpr *PseudoObjectExpr::Create(const ASTContext , Expr *syntax,
@@ -4879,8 +4880,15 @@
Expr *syntax, ArrayRef semantics,
unsigned resultIndex)
 : Expr(PseudoObjectExprClass, type, VK, OK_Ordinary) {
-  PseudoObjectExprBits.NumSubExprs = semantics.size() + 1;
-  PseudoObjectExprBits.ResultIndex = resultIndex + 1;
+
+  if (resultIndex == NoResult) {
+PseudoObjectExprBits.HasResult = false;
+PseudoObjectExprBits.NumSubExprsIfNoResult = semantics.size() + 1;
+  } else {
+PseudoObjectExprBits.HasResult = true;
+PseudoObjectExprBits.ResultBits.NumSubExprs = semantics.size() + 1;
+PseudoObjectExprBits.ResultBits.ResultIndex = resultIndex + 1;
+  }
 
   for (unsigned i = 0, e = semantics.size() + 1; i != e; ++i) {
 Expr *E = (i == 0 ? syntax : semantics[i-1]);
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -591,12 +591,26 @@
 friend class ASTStmtReader; // deserialization
 friend class PseudoObjectExpr;
 
+struct ResultBitfields {
+  // These don't need to be particularly wide, because they're
+  // strictly limited by the forms of expressions we permit.
+  unsigned NumSubExprs : 16;
+  unsigned ResultIndex : 16;
+};
+
 unsigned : NumExprBits;
 
-// These don't need to be particularly wide, because they're
-// strictly limited by 

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

[Yurong via Phabricator via cfe-commits]

yronglin created this revision.
Herald added a project: All.
yronglin requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Signed-off-by: yrong 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154784

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Stmt.h
  clang/lib/AST/Expr.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/test/SemaCXX/builtin-dump-struct.cpp

Index: clang/test/SemaCXX/builtin-dump-struct.cpp
===
--- clang/test/SemaCXX/builtin-dump-struct.cpp
+++ clang/test/SemaCXX/builtin-dump-struct.cpp
@@ -159,3 +159,27 @@
 // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+  v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+  v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+  v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+  v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+  v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+  v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(, printf); }
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1378,8 +1378,15 @@
 void ASTStmtReader::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
   VisitExpr(E);
   unsigned numSemanticExprs = Record.readInt();
-  assert(numSemanticExprs + 1 == E->PseudoObjectExprBits.NumSubExprs);
-  E->PseudoObjectExprBits.ResultIndex = Record.readInt();
+  assert(numSemanticExprs + 1 == E->getNumSubExprs());
+
+  unsigned ResultIndex = Record.readInt();
+  if (ResultIndex) {
+E->PseudoObjectExprBits.HasResult = true;
+E->PseudoObjectExprBits.ResultBits.ResultIndex = ResultIndex;
+  } else {
+E->PseudoObjectExprBits.HasResult = false;
+  }
 
   // Read the syntactic expression.
   E->getSubExprsBuffer()[0] = Record.readSubExpr();
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -4848,7 +4848,8 @@
 
 PseudoObjectExpr::PseudoObjectExpr(EmptyShell shell, unsigned numSemanticExprs)
   : Expr(PseudoObjectExprClass, shell) {
-  PseudoObjectExprBits.NumSubExprs = numSemanticExprs + 1;
+  PseudoObjectExprBits.HasResult = false;
+  PseudoObjectExprBits.NumSubExprsIfNoResult = numSemanticExprs + 1;
 }
 
 PseudoObjectExpr *PseudoObjectExpr::Create(const ASTContext , Expr *syntax,
@@ -4879,8 +4880,15 @@
Expr *syntax, ArrayRef semantics,
unsigned resultIndex)
 : Expr(PseudoObjectExprClass, type, VK, OK_Ordinary) {
-  PseudoObjectExprBits.NumSubExprs = semantics.size() + 1;
-  PseudoObjectExprBits.ResultIndex = resultIndex + 1;
+  
+  if (resultIndex == NoResult) {
+PseudoObjectExprBits.HasResult = false;
+PseudoObjectExprBits.NumSubExprsIfNoResult = semantics.size() + 1;
+  } else {
+PseudoObjectExprBits.HasResult = true;
+PseudoObjectExprBits.ResultBits.NumSubExprs = semantics.size() + 1;
+PseudoObjectExprBits.ResultBits.ResultIndex = resultIndex + 1;
+  }
 
   for (unsigned i = 0, e = semantics.size() + 1; i != e; ++i) {
 Expr *E = (i == 0 ? syntax : semantics[i-1]);
Index: clang/include/clang/AST/Stmt.h
===
--- clang/include/clang/AST/Stmt.h
+++ clang/include/clang/AST/Stmt.h
@@ -591,12 +591,26 @@
 friend class ASTStmtReader; // deserialization
 friend class PseudoObjectExpr;
 
+struct ResultBitfields {
+  // These don't need to be particularly wide, because they're
+  // strictly limited by the forms of expressions we permit.
+  unsigned NumSubExprs : 16;
+  unsigned ResultIndex : 16;
+};
+
 unsigned : NumExprBits;
 
-// These don't need to be particularly