[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-08-09 10:49:00
Message-ID: bug-214950-17878-QurNrO3s1G () http ! bugs ! kde ! org/
[Download RAW message or body]

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

--- Comment #9 from Florian Krohm <florian@eich-krohm.de> ---
Thats for the updated version. Here are a few more comments on it:

> Index: exp-floattrap/tests/inf-float.vgtest
> ===================================================================
> --- exp-floattrap/tests/inf-float.vgtest	(revision 0)
> +++ exp-floattrap/tests/inf-float.vgtest	(revision 0)
> @@ -0,0 +1,3 @@
> +prog: inf-float
> +vgopts: --trap-inf=yes

It is recommended to add -q to vgopts unless you're really interested in ERROR
SUMMARY or the boiler plate stuff valgrind writes out at the beginning. And
here we're not. This allows to simplify the filter_stderr script:

> Index: exp-floattrap/tests/filter_stderr
> ===================================================================
> --- exp-floattrap/tests/filter_stderr	(revision 0)
> +++ exp-floattrap/tests/filter_stderr	(revision 0)

...

> +# Remove "exp-floattrap, ..." line up to the following copyright line.
> +sed "/^exp-floattrap/,+2 d" |

This can go away, because of -q (see above)

> 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,11 @@
> +int main(void)
> +{
> +   // make sure that the value computation is not optimized away by the compiler
> +   volatile float zero = 0;
> +   volatile float one = 1;
> +   volatile float inf;
> +
> +   inf = one/zero;

Compiler warns here that value is unused. Did you not see this warning ?

> +
> +   return 0;

Change to return (int)inf;
which fixes the warning.
Likewise in the other tests.

> Index: exp-floattrap/tests/Makefile.am
> ===================================================================
> --- exp-floattrap/tests/Makefile.am	(revision 0)
> +++ exp-floattrap/tests/Makefile.am	(revision 0)
> @@ -0,0 +1,13 @@
> +include $(top_srcdir)/Makefile.tool-tests.am
> +
> +SUBDIRS = .
> +DIST_SUBDIRS = .
> +
> +dist_noinst_SCRIPTS = \
> +        filter_stderr
> +
> +check_PROGRAMS = \
> +	inf-float \
> +	inf-double \
> +	nan-float \
> +	nan-double

This Makefile.am needs work.  If you had run "make regtest" then at the end
there would have been complaints like these:

...checking makefile consistency
exp-floattrap/tests/Makefile.am:1: error: inf-double.stderr.exp is missing in
EXTRA_DIST
exp-floattrap/tests/Makefile.am:1: error: inf-double.vgtest is missing in
EXTRA_DIST
exp-floattrap/tests/Makefile.am:1: error: inf-float.stderr.exp is missing in
EXTRA_DIST
exp-floattrap/tests/Makefile.am:1: error: inf-float.vgtest is missing in
EXTRA_DIST

Use this command: "make post-regtest-checks"  to check whether your makefiles
are OK.  make post-regtest-checks is run at the end of make regtest.

Then there is this warning:

...checking header files and include directives
Unknown directory 'exp-floattrap'. Please update check_headers_and_includes

You need to update that script. It's obvious what to do once you look at it.

As a final check, you need to run

make dist BUILD_ALL_DOCS=no 

 in the root of the source tree. This command builds a distribution tarball
 and, obviously, it should succeed. 


> Index: exp-floattrap/tests/inf-float.stderr.exp

The .exp files need to be updated due to the  -q  addition in the .vgtest
files.


> Index: exp-floattrap/ft_main.c
> ===================================================================
> --- exp-floattrap/ft_main.c	(revision 0)
> +++ exp-floattrap/ft_main.c	(revision 0)
> @@ -0,0 +1,700 @@

> +   This file is part of FloatTrap, the syncronous floating point trap tracer.

This file is part of FloatTrap, a synchronous floating point trap tracer.

> +
> +/*--------------------------------------------------------------------*/
> +/*--- FloatTrap: The floating point trap.                ft_main.c ---*/

I think you want to say:

FloatTrap: A floating point trap detector.


> +static void ft_tool_error_pp(const Error* e)
> +{
> +   VG_(message)(Vg_UserMsg, "%s:", VG_(get_error_string)(e));
> +   VG_(pp_ExeContext)(VG_(get_error_where)(e));
> +}
> +
> +static UInt ft_tool_error_update_extra(const Error* e)
> +{
> +   return 0;
> +}
> +
> +static Bool ft_tool_error_recog(const HChar* name, Supp* supp)
> +{
> +   return True;
> +}
> +
> +static Bool ft_tool_error_read_extra(Int fd, HChar** buf, SizeT *s, Int *i, Supp* supp)
> +{
> +   return True;
> +}
> +
> +static Bool ft_tool_error_matches(const Error* e, const Supp* supp)
> +{
> +   return True;
> +}
> +
> +static const HChar* ft_tool_error_name(const Error* e)
> +{
> +   switch (VG_(get_error_kind)(e)) {
> +   case NaNResult:
> +      return "NaN error";
> +   case InfResult:
> +      return "Inf error";
> +   default:
> +      return "unknown";
> +   }
> +}
> +
> +static SizeT ft_tool_error_get_extra_si(const Error* e, HChar * c, Int i)
> +{
> +   return 0;
> +}
> +
> +static SizeT ft_tool_error_print_extra_su(const Supp* supp, HChar * c, Int i)
> +{
> +   return 0;
> +}
> +
> +static void ft_tool_error_update_extra_su(const Error* e, const Supp* supp)
> +{
> +}
> +

I'm not sure the above is all that is needed. 
For example suppressing an error does not work. I tried this in
exp-floattrap/tests:

../../vg-in-place --gen-suppressions=all --tool=exp-floattrap ./nan-float

That runs into this assertion:
valgrind: m_errormgr.c:405 (gen_suppression): Assertion 'xtra[num_written] ==
'\0'' failed.


> +static void ft_print_usage(void)
> +{  
> +   VG_(printf)(
> +               "    --trap-inf=yes|no     trap if the result of a float operation is inf\n"
> +               "    --trap-nan=yes|no     trap if the result of a float operation is nan\n"
> +               );

Add the default value. Here is an example:

--show-below-main=no|yes  continue stack traces below main() [no]


> +}
> +
> +static void ft_print_debug_usage(void)
> +{  
> +   VG_(printf)(
> +               "    (none)\n"
> +               );

Write this in a single line. I know you've seen it done differently elsewhere.
But that code was written when computer displays had a 4:3 aspect ratio  :)


Change this:
> +                          True,
to
                             True,/*show TIDs for errors*/

> +/* Add an unary operation to the basic block */
> +
> +static IRTemp addUnop(IRSB* bb, IRType ty, IROp op, IRExpr* arg)
> +{

I'll look at the VEX related stuff in the next iteration in more detail.

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