[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 via KDE Bugzilla <bugzilla_noreply () k
Date:       2015-10-17 16:37:21
Message-ID: bug-214950-17878-KxokPYVYfv () http ! bugs ! kde ! org/
[Download RAW message or body]

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

--- Comment #13 from Mathias Fröhlich <Mathias.Froehlich@gmx.net> ---

Hi,

(In reply to Florian Krohm from comment #12)
> sorry for getting back to you so late...
Same here...

> Thanks for the new patch. Generating suppression business is working now.
> Good. Using them however is not working.
Well, looking at what I have to do here to make it work. I guess I have just
omitted
doing suppressions back these years ago.
You will get that with the next patch.

> 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.
So, this really exists? You made a comment with the wide monitors that
made me think that I do not have to care anymore for 80 chars.

> 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.

Hmm, this may be fuzzy in the end.
At least I expect this to be compiler, compile flags and architecture
dependent.
So, old 80 bits float registers on i386 probably translate into what you
expect.
But what you see on 64 bits intel is the simd variant that works on the lower
part
of the vector register but mentions the whole register. That somehow matches
the F0x2 operation variant somehow. At least this is my expected match of
assembly concepts to the iop descriptions I have found. 
That said, I think we have no clear mapping from c code to the assembler. If we
really want to test each iop separately and mention them in the test code, I
can only see writing everything in architecture dependent assembly.
Do we want purely assembler written tests to nail down the ops we get?

> 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.
Ok, will do.

> 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.
Well, you wanted more documentation and this part is about the only code that
is not just a direct match of widely known cpu concepts and its straight
forward applications. 
But yes, we can cross reference them.

> 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.
Well, isnan is your friend :-)
Seriously, I don't know what I really looked at some years ago. But the
generic, architecture independent isnan implementation looks quite similar even
these days. I will drop a reference to this in the code ...

> 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. 

I know that there is room for improvements in error reporting. 
And yes, I believe it's at first sadly architecture dependent.

Ok, value tracking - the brute force method here.
I don't know enough of valgrind to see if this is easily possible.
Probably we would have to do something similar than memcheck (probably) does.
I think in memcheck must be a flag for each value we process that marks if the
value got
initialized or not. That flag is then propagated across the operations and
evaluated once
we hit a conditional jump. Somehow simplified probably, but basically true??
If we do this with an 'already reported' flag we could be safe.
I fear that this is a lot of work to be done.

Less brute force, we may be able to analyse the inputs and just report an
output value
of NaN if it is not just a simple propagation of a NaN already available in the
input.
Let me think about that a bit more ...
Inf values may be more complex with this idea.

If we can get some of that working, we do not need to go all the architecture
dependent tests then ...

Have a nice weekend ...

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]