[PATCH] D70539: [clang][CodeGen] Implicit Conversion Sanitizer: handle increment/derement (PR44054)
Roman Lebedev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 21 07:30:51 PST 2019
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
More information about the cfe-commits
mailing list