[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-09-10 14:37:06
Message-ID: bug-214950-17878-fENBxwK3DI () http ! bugs ! kde ! org/
[Download RAW message or body]

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

--- Comment #12 from Florian Krohm <florian@eich-krohm.de> ---
Hi Matthias,

sorry for getting back to you so late...
Thanks for the new patch. Generating suppression business is working now.
Good. Using them however is not working.

../../vg-in-place --tool=exp-floattrap --gen-suppressions=all ./nan-double 2>
SUPP
grep -v ^== SUPP > SUPP1
../../vg-in-place --tool=exp-floattrap --suppressions=SUPP1 ./nan-double

==6817== FATAL: in suppressions file "SUPP1" near line 4:
==6817==    location should be "...", or should start with "fun:" or "obj:"
==6817== exiting now.

I believe this line:
  Result of IEEE double operation is NaN
should not be there. Compare with memcheck suppressions..
When I remove the offending line from the suppression file, the syntax
error will go away, but the complaint is not being suppressed.
Please also make sure that a suppression for an INF value does not suppress a
NAN complaint and vice versa (assuming they occur on the same line).

I spotted a few overly long lines in ft_print_usage and ft_pre_clo_init.
You can use C's string literal concatenation: "foo" " bar"  == "foo bar"
to fix this. Limit is 80 chars per line.

Testcases:
nan-float.c and a few others show 2 errors instead of one. The extra
complaint is likely due to the cast expression. That wouldn't be a 
problem per se. However, on other platforms (I tested s390) there is
only one error because the cast is implemented differently, apparently.
We could add an additional .exp file, of course. But, generally, we'd
like not to special-case stuff if it can be avoided. I suggest to rewrite
the tests like so:

volatile float nanval;

int main(void)
{
   /* make sure that the value computation is not optimized away by the
compiler */
   volatile float zero = 0.0f;

   nanval = zero/zero;

   return 0;
}

Note that the global variable cannot be called nan as that conflicts with
a gcc built-in function.

As you'll be editing the testcases anyhow, can you 
(a) fix the overly long comment line  and
(b) add a comment that says which IROp is being tested here ?

I had expected the IROp to be Iop_DivF32 but that wasn't it.
Instead if was an Iop_Div64F0x2 (on x86-64) which was a surprise (to me at
least). On s390 it *is* a Iop_DivF32.
That gives us some coverage for function insert_check. Ideally we'd like to
exercise one IROp
for each block of 'case' labels in that function.

Now: The testcases with inline assembler are specific to x86-64 and won't
compile on other platforms. Move them into exp-floattrap/tests/amd64.
Those tests using "long double" you can copy (not move) into
exp-floattrap/tests/s390x. That way we can test 128-bit wide floating point
ops. Only s390 has that for now.
Don't forget to update configure.ac


ft_main.c:

static void insert_check_float_F16(IRSB* bb, IRTemp value)
{
   /* Blow up the value to a F32 in the hope that this preserves inf and nan
    */

Let's not assume that. This F16 stuff seems to be ARM specific. Perhaps
somebody who knows about ARM can chime in and provide some details.
Until then I'd say, issue a warning (once) when encountering an F16 
operation and continue without instrumenting.

Please define symbolic names for 0x7f800000u and its friends:
#define NAN32 0x7f800000u
or whatever that value is. That will improve readability and avoid long lines.

It is not necessary to explan the "neat trick" three times. Once is enough
and you can have the other places refer to that instance.
I would appreciate a pointer to the place where you found it. You said
earlier it was in glibc somewhere? glibc version and file name would be good.

One more thing came to mind. How difficult would it be to be more specific
in the error message. For example, instead of:

   Result of IEEE double operation is NaN:

say this:

   Result of IEEE double division is NaN:

or multiplication, square root, etc...
This may be helpful when you have a line with lots of floating point
operations. 
because the IR level we're
looking at here is not full trees, but SSA form. But still...

Another thing I noticed is avalanche errors:

For   value = 200 + zero/zero;   there will be two errors: one for the
division and one for the addition. That can be noisy. Given the level the
IR is at at this point (SSA form and *not* trees) this will be quite
difficult to do (if it can be done reasonably at all). I just wanted to
throw it out. Perhaps you can think of something clever.
Here is a difficult example:

      value = exp1 + exp2;

where exp1 and exp2 are expressions both producing a NaN. So you want
an error for the NaN produced by exp1 and one for the NaN produced by
exp2 but no error for the addition of the two. 

When you add a new version of the patch can you mark the previous versions as
obsolete?
Thanks.  Enough for today..

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