Re: [Valgrind-developers] questions on suppression mechanism

[Nicholas N. <n.n...@gm...>]

On Tue, Feb 10, 2009 at 12:51 AM, Konstantin Serebryany
<kon...@gm...> wrote:
>
> First:  I've hit a performance issue in  VG_(maybe_record_error).
> I was running a test that uses openssl library.
> This library has a hand-written assembly file which is not friendly to
> valgrind (valgrind produces broken stack traces, see the recent
> discussion at http://sourceforge.net/mailarchive/forum.php?thread_name=59e975c10902052331r2bf43900qec86f336cba6e531%40mail.gmail.com&forum_name=valgrind-developers).
> This assembly file results in memcheck warnings, like this:
> ==27019== Use of uninitialised value of size 8
> ==27019== at 0xBD3214F: bn_mul_mont
> ==27019== by 0x7A2C730C750BD1E8: ???
> ==27019== by 0xD075D9E9E5D0101: ???
> ==27019== by 0x4EAF8BD8815341D7: ???
> Since the stack traces are broken, each warning found by memcheck is distinct.
> Memcheck boils out after 1000-th report.
>
> So, I needed to suppress these reports.
> {
>   fun:bn_mul_mont
>   Memcheck:Value8
>   fun:bn_mul_mont
> }
>
> As the result, memcheck is slowed down by >100x (from 1 minute to few hours).
> Since all the errors found by memcheck are different,
> VG_(maybe_record_error) tries to add each of them to the list
>   static Error* errors = NULL;
> Since all these errors are suppressed, memcheck does not boil out
> after 1000-th and goes quadratic.
>
> The simple fix which seems to work for me is to check
> is_suppressible_error before checking the 'errors' list.
>
> *** 556,561 ****
> --- 556,569 ----
>     em_errlist_searches++;
>     p       = errors;
>     p_prev  = NULL;
> +
> +    Supp *supp = is_suppressible_error(&err);
> +    if (supp) {
> +       n_errs_suppressed++;
> +       supp->count++;
> +       return ERROR_IS_SUPPRESSED;
> +    }
> +
>     while (p != NULL) {
>        em_errlist_cmps++;
>        if (eq_Error(exe_res, p, &err)) {
>
> Is there any hidden reason not to do that?

This means that is_suppressible_error occurs before the tool has had a
chance to update the 'extra' field.  In some cases Memcheck uses the
'extra' field to decide whether an error matches a suppression.  With
this change the 'extra' field won't have been initialised, which I
think may cause crashes or similar behaviour.

Also, for those suppressions that don't cause a crash, no record will
be made of the suppression, so the list of suppression records printed
at the end with -v will be incomplete.

I can't see a good way to preserve existing behaviour while also
fixing your problem.  I wonder if including suppressed errors in the
don't-report-errors-after-too-many-have-been-found count is the best
approach.

Nick