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 check and this > > check. I think you should aim for exactly one of those checks to fire for > > any given integer conversion. There are the following cases: > > > > * Dst is smaller than Src: if the value changes at all (with sign change > > or without), then the truncation check already catches it, and catching it > > here does not seem useful > > * Dst is the same size as Src or larger: sign change is the only problem, > > and is only possible if exactly one of Src and Dst is signed > > > > So I think you should bail out of this function if either Src and Dst are > > both unsigned or both are signed, and also if Src is larger than Dst > > (because we treat that case as a lossy truncation rather than as a sign > > change). > > > > And when you do emit a check here, the only thing you need to check is if > > the signed value is negative (if so, you definitely changed the sign, and > > if not, you definitely didn't -- except in the truncation cases that the > > truncation sanitizer catches). > To be clear: we want to skip emitting in those cases if the other check > (truncation) is enabled, right? > It does seem to make sense, (and i did thought about that a bit), but i need > to think about it more.. I think we want to skip emitting those checks always (regardless of whether the other sanitizer is enabled). One way to think of it: this sanitizer checks for non-truncating implicit integer conversions that change the value of the result. The other sanitizer checks for truncating implicit integer conversions that change the value of the result. I don't see any point in allowing the user to ask to sanitize sign-changing truncation but not other value-changing truncations. That would lead to this: ``` int a = 0x17fffffff; // no sanitizer warning int b = 0x180000000; // sanitizer warning int c = 0x1ffffffff; // sanitizer warning int d = 0x200000000; // no sanitizer warning ``` ... which I think makes no sense. ================ 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; + // Get the zero of the same type with which we will be comparing. ---------------- lebedev.ri wrote: > rsmith wrote: > > If `!VSigned`, the result is a constant `false`; you don't need to emit an > > `icmp` to work that out. > Ok, if you insist. > I didn't do that in the first place because we will now have an `icmp` > where one operand being a constant, so we can simplify it further. > And i don't want to complicate this logic if middle-end already handles it :) This becomes a lot simpler with the approach I described in the other comment thread, because you don't need a second `icmp eq` at all. 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
