llvm dev - May 2017 - Comparison with Statistic objects

We recently found a bug in an internal pass where we compared a Statistic
object against another value that resulted in a mismatch in output between
debug and release builds. This is because Statistic objects are designed to
be compiled out of release builds. So their increment operator doesn't do
anything and any attempt to read its value returns 0 in release builds.

Another thing to note is that Statistic objects are global variables that
count across multiple pass runs and never reset.

After noticing this behavior we did an audit of trunk to see if we could
find any cases of similar mistakes. We did this by marking all comparison
operators on the Statistic object as deleted. In total we found 6 files in
trunk that failed to build after this.

lib/Target/ARM/ARMOptimizeBarriersPass.cpp had a return value from
runOnMachineFunction dependent on a compare of a Statistic object with 0.
This would always return false in a release build, and always return true
for any pass run after the first pass run that incremented the statistic.
I've fixed this one in r302450 as it was definitely the most suspicious.

lib/CodeGen/TailDuplicator.cpp:257 has a compare between a statistic and a
command line option. I think this makes the command line option broken in
release builds. In debug builds, probably is also applying the command line
option commulatively across each function the pass runs on.

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:339,359,730 have
more compares between command line options and statistics.

lib/Target/ARM/MLxExpansionPass.cpp:214 has another compare between
statistic and command line argument.

The last 3 don't appear to be a functional problem:

lib/Analysis/CallGraphSCCPass.cpp:481 has a compare in order to update a
"max" statistic.

tools/clang/lib/StaticAnalysis/Frontend/AnalysisConsumer.cpp:679 has a
compare for a max statistic.

tools/clang/lib/StaticAnalysis/Frontend/AnalysisConsumer.cpp:568 has a
compare to avoid a divide by 0 in calculating another statistic.


Should we do something about the first set of cases? Should we do something
to prevent creating more such issues in the future by deleting the
comparison operators and providing hooks specifically for the 3 ok cases?

~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170508/090a70e6/attachment.html>

IMHO, we should delete the comparison operators, or make STATISTIC work in
release mode.
(I have trouble thinking they are so slow that it's worth compiling them
out)



On Mon, May 8, 2017 at 11:36 AM, Craig Topper via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> We recently found a bug in an internal pass where we compared a Statistic
> object against another value that resulted in a mismatch in output between
> debug and release builds. This is because Statistic objects are designed to
> be compiled out of release builds. So their increment operator doesn't
do
> anything and any attempt to read its value returns 0 in release builds.
>
> Another thing to note is that Statistic objects are global variables that
> count across multiple pass runs and never reset.
>
> After noticing this behavior we did an audit of trunk to see if we could
> find any cases of similar mistakes. We did this by marking all comparison
> operators on the Statistic object as deleted. In total we found 6 files in
> trunk that failed to build after this.
>
> lib/Target/ARM/ARMOptimizeBarriersPass.cpp had a return value from
> runOnMachineFunction dependent on a compare of a Statistic object with 0.
> This would always return false in a release build, and always return true
> for any pass run after the first pass run that incremented the statistic.
> I've fixed this one in r302450 as it was definitely the most
suspicious.
>
> lib/CodeGen/TailDuplicator.cpp:257 has a compare between a statistic and
> a command line option. I think this makes the command line option broken in
> release builds. In debug builds, probably is also applying the command line
> option commulatively across each function the pass runs on.
>
> lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:339,359,730 have
> more compares between command line options and statistics.
>
> lib/Target/ARM/MLxExpansionPass.cpp:214 has another compare between
> statistic and command line argument.
>
> The last 3 don't appear to be a functional problem:
>
> lib/Analysis/CallGraphSCCPass.cpp:481 has a compare in order to update a
> "max" statistic.
>
> tools/clang/lib/StaticAnalysis/Frontend/AnalysisConsumer.cpp:679 has a
> compare for a max statistic.
>
> tools/clang/lib/StaticAnalysis/Frontend/AnalysisConsumer.cpp:568 has a
> compare to avoid a divide by 0 in calculating another statistic.
>
>
> Should we do something about the first set of cases? Should we do
> something to prevent creating more such issues in the future by deleting
> the comparison operators and providing hooks specifically for the 3 ok
> cases?
>
> ~Craig
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170508/cc93c661/attachment.html>