[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:       Mathias Fröhlich <Mathias.Froehlich () gmx ! net>
Date:       2015-07-31 19:00:24
Message-ID: bug-214950-17878-4K36xeo3tq () http ! bugs ! kde ! org/
[Download RAW message or body]

https://bugs.kde.org/show_bug.cgi?id=214950

--- Comment #5 from Mathias Fröhlich <Mathias.Froehlich@gmx.net> ---
Hi Florian,

Thanks for the review!

On Wednesday, July 29, 2015 21:31:38 Florian Krohm wrote:
> --- Comment #4 from Florian Krohm <florian@eich-krohm.de> ---
> Couple of comments.
I have changed most of what you requested.
For the ones I left out, there is a comment below.

> 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.
The test cases were even outdated a lot.
I have at least brought the current ones into a better shape.
I would agree on the variety of test cases.
Nevertheless, I have not yet(!?) provided these additional cases.

> 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..
Also, changed this a lot. But any suggestions/improvements are welcome. I am no
English
native and I am pretty sure my English wording can stand a lot of improvements.

> +   This file is part of FloatTrap, the syncronous floating point trap tracer.
> 
> the -> a
> synchronous
Well, it is synchronous - that is the whole point *not* to be *A*synchronous.

> +{
> +  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.
That's just +inf as a bit pattern here.
Even if the constant appears somewhere else, this pattern means something
different in the other places. So for example below, when detecting nan's
the same value appears but not really in it's meaning as +inf. Rather than
that,
it does just what the comment says: nifty bit fiddling so that we find out
what we need. BTW, this check for nan is taken from glibc's isnan function.

> +  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.
When I see c++ comments, I would think that valgrind wants c comments, right?

> +#if 0
> +    // Would be easiest, but valgrind issues an error if I do so ...
> 
> Can you elaborate what the problem is?  
No, I don't remember. The original version dates back to 2008 I think ...
It looks like we don't need that anymore.

> IRExpr_Const(IRConst_U64(0x7ff0000000000000ll)));
> 
> What does this large constant represent? 
In this case +inf as bit pattern for double.
I have added a comment like above.

> +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.
Twice, yes, once on the high and once on the low part.
Actually, that is something I was wondering about. I was looking for a
conversion
operator from F128 to I128 so that I can do the same bitmask trick like the
others precisions do. But I could not find such a conversion.
So how is that F128 supposed to work with VEX? Is this the usual doubledouble
approach or is this a real 128 bit IEEE float?

> Did you test this?
Just no ...

Is there some more documentation on VEX? I just did match what appears
plausible to me.

> +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_......
Hmm, I don't think this works the same as this happens in a loop and might
reset the value to False if one of the later conversions did not
result into a modification.
I did use
  modified = insert_check...(blah) || modified;
for some OpenGL piglit tests recently, but I am not sure if this is
really better?
Anyway, these code pieces could stand some restructuring but the point in the
past
provided patch was to have something that works for the ones that asked.

So, having said that, if there is a chance for this to go upstream, I can
polish whatever
you want. But if not, I have to realize that there is life beyond software
which I enjoy also.
:-)

Greetings

Mathias

-- 
You are receiving this mail because:
You are watching all bug changes.=
[prev in list] [next in list] [prev in thread] [next in thread]