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
