Re: Add fuzzing coverage support

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

While this has been posted after stage1 closed and I'm not really happy
that it missed the deadline, I'm willing to grant an exception, the patch
is small enough that it is ok at this point of stage3.  That said, next time
please try to submit new features in time.

Are there any plans for GCC 7 for the other -fsanitize-coverage= options,
or are those just LLVM alternatives to GCC's gcov/-fprofile-generate etc.?

On Thu, Dec 03, 2015 at 08:17:06PM +0100, Dmitry Vyukov wrote:
> +unsigned sancov_pass (function *fun)

Formatting:
unsigned
sancov_pass (function *fun)

> +{
> +  basic_block bb;
> +  gimple_stmt_iterator gsi;
> +  gimple *stmt, *f;
> +  static bool inited;
> +
> +  if (!inited)
> +    {
> +      inited = true;
> +      initialize_sanitizer_builtins ();
> +    }

You can call this unconditionally, it will return as the first thing
if it is already initialized, no need for another guard.

> +
> +  /* Insert callback into beginning of every BB. */
> +  FOR_EACH_BB_FN (bb, fun)
> +    {
> +      gsi = gsi_after_labels (bb);
> +      if (gsi_end_p (gsi))
> +        continue;
> +      stmt = gsi_stmt (gsi);
> +      f = gimple_build_call (builtin_decl_implicit (
> +                             BUILT_IN_SANITIZER_COV_TRACE_PC), 0);

I (personally) prefer no ( at the end of line unless really needed.
In this case you can just do:
      tree fndecl = builtin_decl_implicit (BUILT_IN_SANITIZER_COV_TRACE_PC);
      gimple *g = gimple_build_call (fndecl, 0);
which is same number of lines, but looks nicer.
Also, please move also the gsi, stmt and f (better g or gcall)
declarations to the first assignment to them, they aren't used outside of
the loop.

> --- testsuite/gcc.dg/sancov/asan.c	(revision 0)
> +++ testsuite/gcc.dg/sancov/asan.c	(working copy)
> @@ -0,0 +1,21 @@
> +/* Test coverage/asan interaction:
> +     - coverage instruments __asan_init ctor (thus 4 covarage callbacks)
> +     - coverage does not instrument asan-emitted basic blocks
> +     - asan considers coverage callback as "nonfreeing" (thus 1 asan store
> +       callback.  */
> +/* { dg-do compile } */
> +/* { dg-options "-fsanitize-coverage=trace-pc -fsanitize=address" } */
> +
> +void notailcall ();
> +
> +void foo(volatile int *a, int *b)
> +{
> +  *a = 1;
> +  if (*b)
> +    *a = 2;
> +  notailcall ();
> +}
> +
> +/* { dg-final { scan-assembler-times "call	__sanitizer_cov_trace_pc" 4 } } */
> +/* { dg-final { scan-assembler-times "call	__asan_report_load4" 1 } } */
> +/* { dg-final { scan-assembler-times "call	__asan_report_store4" 1 } } */

I don't like these, we have lots of targets, and different targets have
different instructions for making calls, different whitespace in between
the insn name and called function, sometimes some extra decoration on the fn
name, (say sometimes an extra _ prefix), etc.  IMHO much better to add
-fdump-tree-optimized and scan-tree-dump-times instead for the calls in the
optimized dump.  Affects all tests.

Please repost a patch with these changes fixed, it will be hopefully ackable
then.

	Jakub