This revision was automatically updated to reflect the committed changes.
Closed by commit rL345660: [clang][ubsan] Implicit Conversion Sanitizer -
integer sign change - clang part (authored by lebedevri, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
lebedev.ri updated this revision to Diff 171688.
lebedev.ri added a comment.
Rebased, NFC.
Repository:
rC Clang
https://reviews.llvm.org/D50250
Files:
docs/ReleaseNotes.rst
docs/UndefinedBehaviorSanitizer.rst
include/clang/Basic/Sanitizers.def
lib/CodeGen/CGExprScalar.cpp
lebedev.ri updated this revision to Diff 170952.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.
In https://reviews.llvm.org/D50250#1273415, @rsmith wrote:
> Just some minor nits.
YES! Thank you for the [long-awaited] review!
Addressed review notes.
The compiler-rt
lebedev.ri added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1036
+return;
+ // That's it. We can't rule out any more cases with the data we have.
+
rsmith wrote:
> lebedev.ri wrote:
> > rsmith wrote:
> > > rsmith wrote:
> > > > lebedev.ri
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Just some minor nits.
Comment at: docs/ReleaseNotes.rst:186
+
+ Just like other ``-fsanitize=integer`` checks, these issues are **not**
+ undefined behaviour. But they are
lebedev.ri added a comment.
@rsmith ping.
Repository:
rC Clang
https://reviews.llvm.org/D50250
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
Ping.
The prerequisite "split truncation sanitizer into unsigned and signed cases"
has landed.
I believe i have replied/addressed all the points previously raised here.
Would be awesome to get this going at long last :)
Repository:
rC Clang
lebedev.ri added a comment.
Ping.
Repository:
rC Clang
https://reviews.llvm.org/D50250
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
@rsmith Ping.
Though, https://reviews.llvm.org/D50901 is less controversial, so maybe best to
start there..
Repository:
rC Clang
https://reviews.llvm.org/D50250
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
lebedev.ri added a comment.
In https://reviews.llvm.org/D50250#1236478, @rjmccall wrote:
> It might help if you're more specific about whose review you're asking for.
I suppose the main suspect is still the @rsmith.
Though, https://reviews.llvm.org/D50901 is less controversial, so maybe best
rjmccall added a comment.
It might help if you're more specific about whose review you're asking for.
Repository:
rC Clang
https://reviews.llvm.org/D50250
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
lebedev.ri added a comment.
Ping once again :)
Repository:
rC Clang
https://reviews.llvm.org/D50250
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
Ping.
Repository:
rC Clang
https://reviews.llvm.org/D50250
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
ping.
Repository:
rC Clang
https://reviews.llvm.org/D50250
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1050-1051
+// NOTE: if it is unsigned, then the comparison is naturally always
'false'.
+llvm::ICmpInst::Predicate Pred =
+VSigned ? llvm::ICmpInst::ICMP_SLT : llvm::ICmpInst::ICMP_ULT;
+
lebedev.ri updated this revision to Diff 162050.
lebedev.ri added a subscriber: chandlerc.
lebedev.ri added a comment.
Rebased ontop of https://reviews.llvm.org/D50901, added
- ``-fsanitize=implicit-integer-arithmetic-value-change``: Catches implicit
conversions that change the
lebedev.ri planned changes to this revision.
lebedev.ri added a comment.
Depends on https://reviews.llvm.org/D50901.
(which should land first, ideally.)
Repository:
rC Clang
https://reviews.llvm.org/D50250
___
cfe-commits mailing list
lebedev.ri updated this revision to Diff 160458.
lebedev.ri added a comment.
Ping.
Rebased, now that the https://reviews.llvm.org/D50465 has landed, and we are
now able to properly optimize the ugliest case:
> This comes with `Implicit Conversion Sanitizer - integer sign change`
>
lebedev.ri updated this revision to Diff 159230.
lebedev.ri added a comment.
Do not emit sign-change check in `signed int -> signed char` case.
The truncation check is sufficient:
https://godbolt.org/g/r1wgQG
https://rise4fun.com/Alive/ifj
The middle-end [clearly] does not understand that,
but
lebedev.ri added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1129-1130
+ return;
+ }
+ // That's it. We can't rule out any more cases with the data we have.
Actually, after messing with souper a little, if we are converting from
*larger*
lebedev.ri added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1036
+return;
+ // That's it. We can't rule out any more cases with the data we have.
+
rsmith wrote:
> rsmith wrote:
> > lebedev.ri wrote:
> > > rsmith wrote:
> > > > I don't like
lebedev.ri updated this revision to Diff 159187.
lebedev.ri marked 11 inline comments as done.
lebedev.ri added a comment.
Address most of @rsmith review notes.
Repository:
rC Clang
https://reviews.llvm.org/D50250
Files:
docs/ReleaseNotes.rst
docs/UndefinedBehaviorSanitizer.rst
rsmith added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1036
+return;
+ // That's it. We can't rule out any more cases with the data we have.
+
rsmith wrote:
> lebedev.ri wrote:
> > rsmith wrote:
> > > I don't like the overlap between the
rsmith added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1036
+return;
+ // That's it. We can't rule out any more cases with the data we have.
+
lebedev.ri wrote:
> rsmith wrote:
> > I don't like the overlap between the implicit truncation
lebedev.ri added a comment.
@erichkeane, @rsmith thanks for taking a look!
Comment at: lib/CodeGen/CGExprScalar.cpp:1036
+return;
+ // That's it. We can't rule out any more cases with the data we have.
+
rsmith wrote:
> I don't like the overlap between
rsmith added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1036
+return;
+ // That's it. We can't rule out any more cases with the data we have.
+
I don't like the overlap between the implicit truncation check and this check.
I think you should
lebedev.ri added a project: clang.
lebedev.ri added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1008
+ // We ignore conversions to/from pointer and/or bool.
+ if (!(SrcType->isIntegerType() && DstType->isIntegerType()))
+return;
erichkeane
erichkeane added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1008
+ // We ignore conversions to/from pointer and/or bool.
+ if (!(SrcType->isIntegerType() && DstType->isIntegerType()))
+return;
lebedev.ri wrote:
> erichkeane wrote:
> > I'd
lebedev.ri added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1008
+ // We ignore conversions to/from pointer and/or bool.
+ if (!(SrcType->isIntegerType() && DstType->isIntegerType()))
+return;
erichkeane wrote:
> I'd rather !SrcType->isInt
lebedev.ri updated this revision to Diff 159004.
lebedev.ri marked 5 inline comments as done.
lebedev.ri added a comment.
Address most of @erichkeane review notes.
Repository:
rC Clang
https://reviews.llvm.org/D50250
Files:
docs/ReleaseNotes.rst
docs/UndefinedBehaviorSanitizer.rst
erichkeane added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:2004
+if (auto *ICE = dyn_cast(CE)) {
+ if (CGF.SanOpts.hasOneOf(SanitizerKind::ImplicitConversion) &&
+ !ICE->isPartOfExplicitCast()) {
lebedev.ri wrote:
> erichkeane
lebedev.ri added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:2004
+if (auto *ICE = dyn_cast(CE)) {
+ if (CGF.SanOpts.hasOneOf(SanitizerKind::ImplicitConversion) &&
+ !ICE->isPartOfExplicitCast()) {
erichkeane wrote:
> Is this an
erichkeane added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1008
+ // We ignore conversions to/from pointer and/or bool.
+ if (!(SrcType->isIntegerType() && DstType->isIntegerType()))
+return;
I'd rather !SrcType->isInt || !DestType->isInt
lebedev.ri created this revision.
lebedev.ri added reviewers: vsk, rsmith, rjmccall, Sanitizers.
lebedev.ri added a project: Sanitizers.
C and C++ are interesting languages. They are statically typed, but weakly.
The implicit conversions are allowed. This is nice, allows to write code
while
34 matches
Mail list logo
