[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-bugs-dist
Subject: [valgrind] [Bug 214950] New tool exp-floattrap
From: Florian Krohm <florian () eich-krohm ! de>
Date: 2015-07-29 21:31:38
Message-ID: bug-214950-17878-NcXWnLqanO () http ! bugs ! kde ! org/
[Download RAW message or body]
https://bugs.kde.org/show_bug.cgi?id=214950
Florian Krohm <florian@eich-krohm.de> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |florian@eich-krohm.de
--- Comment #4 from Florian Krohm <florian@eich-krohm.de> ---
Couple of comments.
I've built this on x86-64 with GCC. There are compiler warnings such as this
one:
ft_main.c:130:13: warning: function declaration isn't a prototype
[-Wstrict-prototypes]
static void detected_float_inf()
Use (void) instead.
The testcases have the same issue although there are no complaints because
the compile flags are more permissive there. I suggest you fix those
nevertheless.
Indentation is 3 spaces (and no tabs).
Running the testcases failed. There ought to be a filter_stderr script in the
tests directory.
We would also want more testcases. There should be at least one for every
insert_check_XXXXX function.
Index: exp-floattrap/tests/inf-float.c
===================================================================
--- exp-floattrap/tests/inf-float.c (revision 0)
+++ exp-floattrap/tests/inf-float.c (revision 0)
@@ -0,0 +1,15 @@
+#include <stdio.h>
+
+int main()
+{
+ float zero = 0;
+ float one = 1;
+ float inf;
+
+ inf = one/zero;
Add volatile to the declarations to make sure the computation is not
constant-folded by some clever compiler. Likewise for the other testcases.
Index: exp-floattrap/docs/ft-manual.xml
===================================================================
--- exp-floattrap/docs/ft-manual.xml (revision 0)
+++ exp-floattrap/docs/ft-manual.xml (revision 0)
@@ -0,0 +1,41 @@
+<?xml version="1.0"?> <!-- -*- sgml -*- -->
+<!DOCTYPE chapter PUBLIC "-//OASIS//DTD DocBook XML V4.2//EN"
+ "http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd">
+
+
+<chapter id="ft-manual" xreflabel="Floattrap">
+
+<title>Floattrap: the "floatingpoint trap" tool</title>
+<subtitle>Checks each result of floating point operations for NaN or
+Inf values.</subtitle>
+
+<para>A tool that examines the result of every floating point
+operation and checks these results for NaN or inf values.</para>
+
+<para>Where is it really useful to have floating point values that
+represent invalid numbers like the IEEE NaN value or having values
+where you can represent values beyond the range of floating point
+values, it is also often catastrophic if these values occur without
+being noticed. Often when tese values show up in computational
+programs or simulation codes, NaNs propagate fast through the whole
+simulation program. Typically you will notice them at some values you
+observe past the original problem occured. But tracking the original
+line of code where the first NaN is happens is a huge problem.</para>
+
+<para>There are hardware floating point traps available on most
+cpus. But they usually happen ansyncronous and signal very often
+sometimes past the real problem happened. This really limits the cases
+where you can get useful information from that hardware traps to very
+few cases.</para>
+
Can you rephrase this a bit? The key point here is that you want to
report nans and infs as soon as possible )namely, when they are created)
because
computation with those funny numbers is usually unintended and causes
unexpected output down the road.
I also suggest to spell-check the text. There are some errors..
Index: exp-floattrap/ft_main.c
===================================================================
--- exp-floattrap/ft_main.c (revision 0)
+++ exp-floattrap/ft_main.c (revision 0)
@@ -0,0 +1,736 @@
+
+/*--------------------------------------------------------------------*/
+/*--- FloatTrap: The floating point trap. ft_main.c ---*/
+/*--------------------------------------------------------------------*/
+
+/*
+ This file is part of FloatTrap, the syncronous floating point trap tracer.
the -> a
synchronous
+static void
+insert_check_float_(IRSB* bb, IRTemp value)
For consistency with other functions you defined earlier use
static void insert_check_float_(IRSB* bb, IRTemp value)
+{
+ IRTemp absValue = addBinop(bb, Ity_I32, Iop_And32,
+ IRExpr_RdTmp(value),
+ IRExpr_Const(IRConst_U32(0x7fffffff)));
+
+ if (check_for_inf) {
+ IRTemp isFinite = addBinop(bb, Ity_I1, Iop_CmpEQ32,
+ IRExpr_RdTmp(absValue),
+ IRExpr_Const(IRConst_U32(0x7f800000)));
What is this magic constant? Please #define a symbolic constant.
+
+ IRDirty* dirty = unsafeIRDirty_0_N(0, "detected_float_inf",
+ detected_float_inf, mkIRExprVec_0());
+ dirty->guard = IRExpr_RdTmp(isFinite);
Perhaps that variable should be called isInfinite?
+ addStmtToIRSB(bb, IRStmt_Dirty(dirty));
+ }
+
+ if (check_for_nan) {
+ // Neat trick:
+ // A nan has all bits in its exponent set and any bit in the mantissa.
+ // So a nan's bitpattern int representation can be written as
+ // 0x7f800000 + x
+ // where x > 0.
+ // That means compute:
+ // t = 0x7f800000 - (0x7f800000 + x) = -x
+ // and look for the sign of t.
+ // If the exponent is not 0x7f800000 (that is less than 0x7f800000), the
+ // resulting t value will be >= 0.
+
+ IRTemp nanTestTemp = addBinop(bb, Ity_I32, Iop_Sub32,
+ IRExpr_Const(IRConst_U32(0x7f800000)),
+ IRExpr_RdTmp(absValue));
+
+#if 0
+ // Would be easiest, but valgrind issues an error if I do so ...
Can you elaborate what the problem is?
+static void
+insert_check_double_(IRSB* bb, IRTemp value)
+{
+ IRTemp absValue = addBinop(bb, Ity_I64, Iop_And64,
+ IRExpr_RdTmp(value),
+ IRExpr_Const(IRConst_U64(0x7fffffffffffffffll)));
Use ull suffix here.
+
+ if (check_for_inf) {
+ IRTemp isFinite = addBinop(bb, Ity_I1, Iop_CmpEQ64,
+ IRExpr_RdTmp(absValue),
+
IRExpr_Const(IRConst_U64(0x7ff0000000000000ll)));
and here.
What does this large constant represent?
+
+static void
+insert_check_double128(IRSB* bb, IRTemp value)
+{
+ IRTemp tempHi, tempLo;
+
+ tempHi = addUnop(bb, Ity_F64, Iop_F128HItoF64, IRExpr_RdTmp(value));
+ insert_check_double(bb, tempHi);
+
+ tempLo = addUnop(bb, Ity_F64, Iop_F128LOtoF64, IRExpr_RdTmp(value));
+ insert_check_double(bb, tempLo);
So you're using insert_check_double twice. I doubt that is correct.
Did you test this?
+static
+IRSB* ft_instrument ( VgCallbackClosure* closure,
+ IRSB* bb_in,
+ const VexGuestLayout* layout,
+ const VexGuestExtents* vge,
+ const VexArchInfo* archinfo_host,
+ IRType gWordTy, IRType hWordTy )
+{
+ Int i;
+ IRSB* bb;
+ Bool modified = False;
+
+ bb = deepCopyIRSBExceptStmts(bb_in);
+ for (i = 0; i < bb_in->stmts_used; i++) {
+ IRStmt* stmt;
+ IRType tempType;
+
+ /* Append the current statement */
+ stmt = deepCopyIRStmt(bb_in->stmts[i]);
+ addStmtToIRSB(bb, stmt);
+
+ /* Ok, we have an expression with a result */
+ if (stmt->tag != Ist_WrTmp)
+ continue;
+
+ /* We are only interrested in floaintg point assignemnt expressions */
+ tempType = typeOfIRTemp(bb_in->tyenv, stmt->Ist.WrTmp.tmp);
+ if (tempType == Ity_F16) {
+ /* Results from floating point ops are always checked */
+ IRExpr* rhs = stmt->Ist.WrTmp.data;
+ switch (rhs->tag) {
+ case Iex_Qop:
+ case Iex_Triop:
+ case Iex_Binop:
+ case Iex_Unop:
+ insert_check_float16(bb, stmt->Ist.WrTmp.tmp);
+ modified = True;
Below you do:
if (insert_check_.........)
modified = True;
So this here is inconsistent.
You can also simply say:
modified = insert_check_......
More occurrences of this below..
+ break;
+ default:
+ break;
+ }
+ }
+ else if (tempType == Ity_F32) {
+ /* Results from floating point ops are always checked */
+ IRExpr* rhs = stmt->Ist.WrTmp.data;
+ switch (rhs->tag) {
+ case Iex_Qop:
+ case Iex_Triop:
+ case Iex_Binop:
+ case Iex_Unop:
+ insert_check_float(bb, stmt->Ist.WrTmp.tmp);
+ modified = True;
+ break;
+ default:
+ break;
+ }
+ }
+ else if (tempType == Ity_F64) {
+ /* Results from floating point ops are always checked */
+ IRExpr* rhs = stmt->Ist.WrTmp.data;
+ switch (rhs->tag) {
+ case Iex_Qop:
+ case Iex_Triop:
+ case Iex_Binop:
+ case Iex_Unop:
+ insert_check_double(bb, stmt->Ist.WrTmp.tmp);
+ modified = True;
+ break;
+ default:
+ break;
+ }
+ }
+ else if (tempType == Ity_F128) {
+ /* Results from floating point ops are always checked */
+ IRExpr* rhs = stmt->Ist.WrTmp.data;
+ switch (rhs->tag) {
+ case Iex_Qop:
+ case Iex_Triop:
+ case Iex_Binop:
+ case Iex_Unop:
+ insert_check_double128(bb, stmt->Ist.WrTmp.tmp);
+ modified = True;
+ break;
+ default:
+ break;
+ }
+ }
+ else if (tempType == Ity_V128) {
+ IRExpr* rhs = stmt->Ist.WrTmp.data;
+ switch (rhs->tag) {
+ case Iex_Qop:
+ break;
+ case Iex_Triop:
+ if (insert_check(bb, rhs->Iex.Triop.details->op, stmt->Ist.WrTmp.tmp))
+ modified = True;
+ break;
+ case Iex_Binop:
+ if (insert_check(bb, rhs->Iex.Binop.op, stmt->Ist.WrTmp.tmp))
+ modified = True;
+ break;
+ case Iex_Unop:
+ if (insert_check(bb, rhs->Iex.Unop.op, stmt->Ist.WrTmp.tmp))
+ modified = True;
+ break;
+
+ default:
+ break;
+ }
+ }
+ else if (tempType == Ity_V256) {
+ IRExpr* rhs = stmt->Ist.WrTmp.data;
+ switch (rhs->tag) {
+ case Iex_Qop:
+ break;
+ case Iex_Triop:
+ if (insert_check(bb, rhs->Iex.Triop.details->op, stmt->Ist.WrTmp.tmp))
+ modified = True;
+ break;
+ case Iex_Binop:
+ if (insert_check(bb, rhs->Iex.Binop.op, stmt->Ist.WrTmp.tmp))
+ modified = True;
+ break;
+ case Iex_Unop:
+ if (insert_check(bb, rhs->Iex.Unop.op, stmt->Ist.WrTmp.tmp))
+ modified = True;
+ break;
+
+ default:
+ break;
+ }
+ }
+ }
+
+ (void)modified;
+ /* if (modified) */
+ /* ppIRSB ( bb ); */
+
if (0 && modified)
is a pattern that we've been using in other places..
+static void ft_pre_clo_init(void)
+{
+ VG_(details_name) ("Exp-Floattrap");
exp-floattrap
--
You are receiving this mail because:
You are watching all bug changes.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic