On Tue, 25 Mar 2014, Jakub Jelinek wrote: > On Tue, Mar 25, 2014 at 10:09:26AM +0100, Richard Biener wrote: > > On Tue, 25 Mar 2014, Jakub Jelinek wrote: > > > > > Hi! > > > > > > While Marek has been debugging while some perl test fails when perl is > > > built > > > with GCC 4.9, we've discovered that it is because of undefined behavior in > > > it: > > > ... > > > && (((UV)1 << NV_PRESERVES_UV_BITS) > > > > (UV)(SvIVX(sv) > 0 ? SvIVX(sv) : -SvIVX(sv))) > > > where SvIVX(sv) can be LONG_MIN, at which point there is undefined > > > behavior > > > on the negation, but -fsanitize=undefined did detect only other issues in > > > the same source file and not this one, because fold-const.c folded it into > > > ABS_EXPR early. > > > > > > This patch disables such folding, because all the A op 0 ? A : -A > > > operations this if is trying to optimize will need instrumentation with > > > -fsanitize=signed-integer-overflow. > > > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > Can't one form a valid constant expression using this? Also isn't > > ABS_EXPR undefined for LONG_MIN as well? So why is ubsan not > > instrumenting that instead? > > Well, for a constant expression A would need to fold to a constant, but then > A op 0 would fold to a constant too and so would the whole A op 0 ? A : -A. > > Instrumenting ABS_EXPR is certainly possible, but definitely more code than > this patch. I could revert this patch and implement it for 5.0 as ABS_EXPR > instrumentation. Of course, if you think we should do the instrumentation > of ABS_EXPR, I can hack it up now. > > I was afraid of also the other transformations in: > A == 0? A : -A same as -A > A != 0? A : -A same as A > A >= 0? A : -A same as abs (A) > A > 0? A : -A same as abs (A) > A <= 0? A : -A same as -abs (A) > A < 0? A : -A same as -abs (A) > but only the second one doesn't add ABS_EXPR and removes the negation, but > in that case the negation would happen only for 0, thus it would be fine.
Yes, all transforms in fold-const would be invalid if the result doesn't behave in the same way wrt overflow. Thus you really should instrument ABS_EXPR - you can treat it as A > 0 ? A : -A if that simplifies it. I don't like the conditions that disable stuff based on sanitization. Instrumenting ABS_EXPR shouldn't be too difficult. Richard.
