lebedev.ri added a comment. Thank you for taking a look!
In D70539#1755121 <https://reviews.llvm.org/D70539#1755121>, @erichkeane wrote: > I haven't looked at the tests because I don't terribly understand the > sanitizer IR (hopefully someone else can take a look), You can ignore the final IR itself in princible. Since this implementation approach avoided re-engineering the checking, we can piggy-back on the expectation that if the sanitization IR was wrong, we'd catch it already (it is not wrong.). So only the `CGExprScalar.cpp` changes need review in principle - the promotion + wide add + demotion. > but the logic/motivation seems solid to me. Yay! :) ================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2422 } else if (type->isIntegerType()) { - // Note that signed integer inc/dec with width less than int can't - // overflow because of promotion rules; we're just eliding a few steps here. - if (E->canOverflow() && type->isSignedIntegerOrEnumerationType()) { + if (CGF.SanOpts.hasOneOf(SanitizerKind::ImplicitIntegerTruncation) && + type->isPromotableIntegerType()) { ---------------- erichkeane wrote: > Should this be 'has' instead of 'hasOneOf'? Uhm, it looks it can be `CGF.SanOpts.has(SanitizerKind::ImplicitSignedIntegerTruncation)`, but then i will need to add a defensive assert. `check-clang` is running.. ================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2428 + // promotion rules, so we omit promotion+demotion, which means that we can + // not catch lossy "demotion". Since we still want to catch those cases, + // if the sanitizer is enabled, let's perform the promotion, perform ---------------- erichkeane wrote: > comma at the end here isn't required. The one after 'enabled' needs to be a > semicolon I think? > comma at the end here isn't required. Right. > The one after 'enabled' needs to be a semicolon I think? It doesn't look like that to me? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70539/new/ https://reviews.llvm.org/D70539 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
