[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

rsmith added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137 + overflow happens (signed or unsigned). + Both of these two issues are handled by ``-fsanitize=implicit-conversion`` + group of checks. - ``-fsanitize=unreachable``: If control

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137 + overflow happens (signed or unsigned). + Both of these two issues are handled by ``-fsanitize=implicit-conversion`` + group of checks. - ``-fsanitize=unreachable``: If

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

This revision was automatically updated to reflect the committed changes. Closed by commit rC338288: [clang][ubsan] Implicit Conversion Sanitizer - integer truncation - clang part (authored by lebedevri, committed by ). Repository: rC Clang https://reviews.llvm.org/D48958 Files:

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:136-137 + overflow happens (signed or unsigned). + Both of these two issues are handled by ``-fsanitize=implicit-conversion`` + group of checks. - ``-fsanitize=unreachable``: If

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 158030. lebedev.ri marked 9 inline comments as done. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Address last portion of @rsmith review notes. @rsmith, @vsk, @erichkeane - thank you for the review! Repository: rC Clang

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

rsmith accepted this revision. rsmith added a comment. Only comments on documentation and assertions. Feel free to commit once these are addressed to your satisfaction. Comment at: docs/ReleaseNotes.rst:310 + + Just like ``-fsanitize=integer``, these issues are **not**

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added a comment. Oops, forgot to submit the inline comments. (It is inconvenient that they aren't submitted with the rest.) Comment at: docs/ReleaseNotes.rst:292 + store = store + 768; // before addition, 'store' was promoted to int. +

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 157665. lebedev.ri marked 9 inline comments as done and 2 inline comments as done. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. Hopefully address @rsmith review notes: - s/cast/conversion/ where appropriate - Some wording in

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

rsmith added inline comments. Comment at: docs/ReleaseNotes.rst:49-51 +- A new Implicit Cast Sanitizer (``-fsanitize=implicit-cast``) group was added. + Please refer to the :ref:`release-notes-ubsan` section of the release notes + for the details. Regarding

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 157585. lebedev.ri added a comment. Rebase, Address @rsmith review notes - just inline https://reviews.llvm.org/D49844. Repository: rC Clang https://reviews.llvm.org/D48958 Files: docs/ReleaseNotes.rst docs/UndefinedBehaviorSanitizer.rst

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:93-97 + - ``-fsanitize=implicit-integer-truncation``: Implicit cast from integer + of bigger bit width to smaller bit width, if that results in data loss. + That is, if the demoted value,

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 157498. lebedev.ri marked 2 inline comments as done. lebedev.ri added a comment. Small rewording in `docs/UndefinedBehaviorSanitizer.rst` thanks to @erichkeane & @aaron.ballman! Repository: rC Clang https://reviews.llvm.org/D48958 Files:

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

aaron.ballman added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:93-97 + - ``-fsanitize=implicit-integer-truncation``: Implicit cast from integer + of bigger bit width to smaller bit width, if that results in data loss. + That is, if the demoted

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

erichkeane accepted this revision. erichkeane added a comment. 1 Nit, otherwise LGTM. Comment at: docs/UndefinedBehaviorSanitizer.rst:95 + of bigger bit width to smaller bit width, if that results in data loss. + That is, if the demoted value, after casting back to the

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: test/CodeGenCXX/catch-implicit-integer-truncations.cpp:8-33 +// == // +// Check that explicit cast does not interfere with implicit cast +//

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 157458. lebedev.ri marked an inline comment as done. lebedev.ri added a reviewer: erichkeane. lebedev.ri added a subscriber: erichkeane. lebedev.ri added a comment. Address @rsmith & @erichkeane [IRC] review notes: - https://reviews.llvm.org/D49838 -

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:979-1003 +bool ScalarExprEmitter::IsTopCastPartOfExplictCast() { + assert(!CastExprStack.empty()); + // Walk the current stack of CastExprs in reverse order. + // That is, the current CastExpr, which

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added a comment. In https://reviews.llvm.org/D48958#1173860, @vsk wrote: > LGTM, although I think it'd be helpful to have another +1 just to be safe. Thank you for the review! It would indeed be great if someone else could take a look, especially since we are **so** close to the

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk accepted this revision. vsk added a comment. This revision is now accepted and ready to land. LGTM, although I think it'd be helpful to have another +1 just to be safe. I did two small experiments with this using a Stage1 Rel+Asserts compiler: Stage2 Rel+Asserts build of llvm-tblgen: ninja

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 156983. lebedev.ri added a comment. Rebased on top of svn tip / git master, now that https://reviews.llvm.org/D49508 has landed, which means there shouldn't be any more false-positives (and it's a bit faster to detect that the check shouldn't be emitted,

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 156583. lebedev.ri marked 11 inline comments as done. lebedev.ri added a comment. Address @vsk's review notes. Repository: rC Clang https://reviews.llvm.org/D48958 Files: docs/ReleaseNotes.rst docs/UndefinedBehaviorSanitizer.rst

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:383 + // This stack is used/maintained exclusively by the implicit cast sanitizer. + llvm::SmallVector CastExprStack; + vsk wrote: > lebedev.ri wrote: > > vsk wrote: > > > lebedev.ri

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:383 + // This stack is used/maintained exclusively by the implicit cast sanitizer. + llvm::SmallVector CastExprStack; + lebedev.ri wrote: > vsk wrote: > > lebedev.ri wrote: > > > vsk wrote: >

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:383 + // This stack is used/maintained exclusively by the implicit cast sanitizer. + llvm::SmallVector CastExprStack; + vsk wrote: > lebedev.ri wrote: > > vsk wrote: > > > Why not 0

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:159 - ``-fsanitize=undefined``: All of the checks listed above other than ``unsigned-integer-overflow`` and the ``nullability-*`` checks. - ``-fsanitize=undefined-trap``: Deprecated alias of

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 156470. lebedev.ri marked 6 inline comments as done. lebedev.ri added a comment. Rebased ontop of yet-again rewritten https://reviews.llvm.org/D49508. Addressed all @vsk's review notes. More review notes wanted :) Repository: rC Clang

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:134 + integer promotions, as those may result in an unexpected computation + results, even though no overflow happens (signed or unsigned). - ``-fsanitize=unreachable``: If control flow

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:97 + is not equal to the original value before the downcast. This kind of issues + may often be caused by an implicit integer promotions. - ``-fsanitize=integer-divide-by-zero``: Integer

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 156237. lebedev.ri added a comment. Breakthrough: no more false-positives due to the `MaterializeTemporaryExpr` skipping over NoOp casts. (https://reviews.llvm.org/D49508) Slight docs update. Ping, please review! We are so close :) Repository: rC

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added a comment. Well, that's just great, with `isCastPartOfExplictCast()`, the `ASTContext::getParents()` also does not return `CXXStaticCastExpr` as parent for such cases. I don't know how to proceed. Repository: rC Clang https://reviews.llvm.org/D48958

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 155363. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. Address @vsk review notes, although this will be revered by the next update dropping the faulty 'stack' optimization. Repository: rC Clang

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri marked 5 inline comments as done. lebedev.ri added a comment. @vsk so yeah, no wonder that doesn't work. Somehow in that test case `ScalarExprEmitter::VisitExplicitCastExpr()` **never** gets called. (I'm pretty sure this worked with the naive implementation, so worst case i'll just

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a comment. In https://reviews.llvm.org/D48958#1160494, @lebedev.ri wrote: > In https://reviews.llvm.org/D48958#1160479, @vsk wrote: > > > In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote: > > > > > Thank you for taking a look! > > > > > > In

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added a comment. In https://reviews.llvm.org/D48958#1160848, @vsk wrote: > In https://reviews.llvm.org/D48958#1160494, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D48958#1160479, @vsk wrote: > > > > > In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote: > > > > > >

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added a comment. In https://reviews.llvm.org/D48958#1160853, @lebedev.ri wrote: > In https://reviews.llvm.org/D48958#1160848, @vsk wrote: > > > <...> > > The stage2 build traps before it finishes: > > > > FAILED: lib/IR/AttributesCompatFunc.inc.tmp > > cd > >

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added a comment. Thank you for taking a look! In https://reviews.llvm.org/D48958#1160381, @vsk wrote: > I have some minor comments but overall I think this is in good shape. It > would be great to see some compile-time numbers just to make sure this is > tractable. I'm pretty sure

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a comment. In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote: > Thank you for taking a look! > > In https://reviews.llvm.org/D48958#1160381, @vsk wrote: > > > I have some minor comments but overall I think this is in good shape. It > > would be great to see some

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a comment. I have some minor comments but overall I think this is in good shape. It would be great to see some compile-time numbers just to make sure this is tractable. I'm pretty sure -fsanitize=null would fire more often across a codebase than this check, so I don't anticipate a

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added a comment. In https://reviews.llvm.org/D48958#1160479, @vsk wrote: > In https://reviews.llvm.org/D48958#1160435, @lebedev.ri wrote: > > > Thank you for taking a look! > > > > In https://reviews.llvm.org/D48958#1160381, @vsk wrote: > > > > > I have some minor comments but overall

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1694 // handle things like function to ptr-to-function decay etc. Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { Expr *E = CE->getSubExpr(); vsk wrote: > lebedev.ri wrote: >

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 155149. lebedev.ri marked 7 inline comments as done. lebedev.ri added a comment. Address @vsk's review notes. - Maintain the stack of currently-being-visited `CastExpr`'s - Use that stack to check whether we are in a `ExplicitCastExpr` - Move logic for

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a subscriber: klimek. vsk added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1694 // handle things like function to ptr-to-function decay etc. Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { Expr *E = CE->getSubExpr();

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 155064. lebedev.ri added a comment. Add some more tricky tests where maintaining just the `CastExpr` part of AST stack would break them. Repository: rC Clang https://reviews.llvm.org/D48958 Files: docs/ReleaseNotes.rst

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1694 // handle things like function to ptr-to-function decay etc. Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { Expr *E = CE->getSubExpr(); vsk wrote: > lebedev.ri wrote: >

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1694 // handle things like function to ptr-to-function decay etc. Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { Expr *E = CE->getSubExpr(); lebedev.ri wrote: > vsk wrote: > > I

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:318 - Value *EmitScalarConversion(Value *Src, QualType SrcTy, QualType DstTy, + Value *EmitScalarConversion(Value *Src, QualType SrcType, QualType DstType,

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

vsk added a comment. Thanks for working on this! Comment at: docs/ReleaseNotes.rst:277 + behaviour. But they are not *always* intentional, and are somewhat hard to + track down. Could you mention whether the group is enabled by -fsanitize=undefined?

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 154877. lebedev.ri marked 2 inline comments as done. lebedev.ri added a comment. @vsk thank you for taking a look! Addressed the trivial part of nits. Repository: rC Clang https://reviews.llvm.org/D48958 Files: docs/ReleaseNotes.rst

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 154863. lebedev.ri added a comment. - Check that sanitizer is actually enabled before doing the AST upwalk. I didn't measure, but it would be logical for this to be better. Repository: rC Clang https://reviews.llvm.org/D48958 Files:

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri added a comment. Finished running it on a normal testset of my pet project . - It fired ~18 times. - There were no obvious false-positives (e.g. when an explicit cast was involved). - At least 3 of those appear to be a true bugs. - 4-5 more

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

lebedev.ri updated this revision to Diff 154591. lebedev.ri retitled this revision from "[private][clang] Implicit Cast Sanitizer - integer truncation - clang part" to "[clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part". lebedev.ri edited the summary of this revision.