Possible clang bug with -O2, wrong if branch entered, in clang version 2.1 (tags/Apple/clang-163.7.1)

Clang Frontend
#1

Hello,

today I tried to compile Redis with clang, and a test was failing
without apparent reasons.
I investigated a bit further, and isolated the bug in the following
code fragment:

https://gist.github.com/187687

I verified the bug only in Apple's clang, I'm not 100% sure this is
the right place where to submit such a bug report. If this is the
wrong place, I'm sorry, please point me in the right direction.

I'm not subscribed to the list so if you need further informations
please include my address in CC.

Thank you in advance,
Salvatore

#2

Sorry, there was a problem with cut&paste of the gist URL, the correct one is:

https://gist.github.com/1876875

Salvatore

#3

Hi Salvatore,

If I understand this code correctly, there is a signed overflow.
Signed overflow is undefined behavior. Clang (or LLVM) apparently
takes advantage of that by optimizing (incr < 0 && value > oldvalue)
=> false.

Dmitri

#4

Sorry, there was a problem with cut&paste of the gist URL, the correct one is:

https://gist.github.com/1876875

You code does signed integer wrap which is undefined behavior.

#5

Hello,

Sorry, there was a problem with cut&paste of the gist URL, the correct one is:

https://gist.github.com/1876875

Salvatore

Hello,

today I tried to compile Redis with clang, and a test was failing
without apparent reasons.
I investigated a bit further, and isolated the bug in the following
code fragment:

https://gist.github.com/187687

I verified the bug only in Apple's clang, I'm not 100% sure this is
the right place where to submit such a bug report. If this is the
wrong place, I'm sorry, please point me in the right direction.

I can confirm the bug on x86_64-apple-darwin10.8.0 using clang r151042 with -O1, -O2, -O3 and -Os.

Bugs should be filed in LLVM's bugtracker. I took the liberty of doing it for you:
<http://llvm.org/bugs/show_bug.cgi?id=12051>

HTH,
Jonathan

#6

And you can get clang to barf on such occurrences:
<http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html>

#7

Hi Salvatore,

If I understand this code correctly, there is a signed overflow.
Signed overflow is undefined behavior. Clang (or LLVM) apparently
takes advantage of that by optimizing (incr < 0 && value > oldvalue)
=> false.

I Dmitri,

I understand the code is undefined, but a compiler can decide to have
different practical behaviors when an undefined condition is
encountered. I think that the best way to handle this condition in the
real world is to do the math accordingly to the underlaying
architecture, so that just the result is undefined, but avoiding to
take advantage of the fact it is undefined to optimized.

I'm not stating that clang is behavior is no correct: strictly
speaking it is following the standard. However its concrete behavior
probably does not minimize the probability of real-world (possibly
broken) code fails. That said I'm going to fix my code so that it is
standard complaint and can compile fine with clang.

Thank you,
Salvatore

#8

Hello,

Sorry, there was a problem with cut&paste of the gist URL, the correct

one is:

https://gist.github.com/1876875

Hi Salvatore,

If I understand this code correctly, there is a signed overflow.
Signed overflow is undefined behavior. Clang (or LLVM) apparently
takes advantage of that by optimizing (incr < 0 && value > oldvalue)
=> false.

And you can get clang to barf on such occurrences:
<http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html>

Is there a reason why integer overflow checks are only inserted with -ftrapv, but not with
-fcatch-undefined-behavior? (clang r151042)

Jonathan

#9

Everything fine after this commit:
https://github.com/antirez/redis/commit/7c96b467c1f882874f80403101ec96ddaf624f1a

Sorry clang!

Thanks for the help,
Salvatore

p.s. clang static checker was able to find a few real bugs in the
Redis source code, and the output is awesome.

#10

Hello Salvatore,

would you mind showing on this list the bugs that the static analyzer found in Redis ?

A few pointers to the patches where those bugs were fixed would be amply sufficient.

– Matthieu

#11

Sure, including today's undefined behavior (not found by the static
analyzer) I fixed three bugs thanks to clang so far:

$ git log --oneline | grep clang
7c96b46 Fixed undefined behavior in *INCR style functions overflow
detection. Sorry clang!
4e97c2c Fixed another possible bug in cluster.c found by clang --analyze.
6710ff2 Fixed a non critical bug signaled by clang static analyzer
thanks to Mukund Sivaraman for reporting it: there was a not
initialized field populating the cluster message header, but it is
always fixed at later time before sending the packet.

You can see patches you can simply go here:

https://github.com/antirez/redis/commit/<SHA1>

For instance: https://github.com/antirez/redis/commit/4e97c2c

Cheers,
Salvatore

#12

Hi,

another real bug found by clang in the Redis code base:

https://github.com/antirez/redis/commit/21661d7acc9ed525c1150f6fa55646d19f5a31d9

Cheers,
Salvatore