On Wed, Dec 02, 2015 at 05:55:29PM +0100, Dmitry Vyukov wrote:
> Can you point to some concrete coding style violations (besides
> function comments)?
>
>
> > We seem to have no established process for deciding whether we want a new
> > feature. I am not sure how to approach such a question, and it comes up
> > quite often. Somehow the default answer usually seems to be to accept
> > anything.
> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 231000)
> +++ ChangeLog (working copy)
> @@ -1,3 +1,13 @@
> +2015-11-27 Dmitry Vyukov <dvyu...@google.com>
> +
> + * sancov.c: add file.
> + * Makefile.in: add sancov.c.
> + * passes.def: add sancov pass.
> + * tree-pass.h: add sancov pass.
> + * common.opt: add -fsanitize-coverage=trace-pc flag.
> + * sanitizer.def: add __sanitizer_cov_trace_pc builtin.
> + * builtins.def: enable sanitizer builtins when coverage is enabled.
All the ChangeLog entries should start with upper case letter after :,
instead of add file. say New file., in most other cases there is missing
() with what you've actually changed.
Say
* Makefile.in (OBJS): Add sancov.o.
You should not add sancov.c to GTFILES if there are no GTY variables (and if
there were, you would need to include the generated gt-* header at the end).
* builtins.def (DEF_SANITIZER_BUILTIN): Enable for
flag_sanitize_coverage.
etc.
> --- builtins.def (revision 231000)
> +++ builtins.def (working copy)
> @@ -210,7 +210,8 @@
> DEF_BUILTIN (ENUM, "__builtin_" NAME, BUILT_IN_NORMAL, TYPE, TYPE, \
> true, true, true, ATTRS, true, \
> (flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \
> - | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT)))
> + | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT) \
> + || flag_sanitize_coverage))
The || should be indented below flag_.
> + FOR_EACH_BB_FN (bb, fun)
> + {
> + gsi = gsi_start_bb (bb);
> + stmt = gsi_stmt (gsi);
> + while (stmt && dyn_cast <glabel *> (stmt))
> + {
> + gsi_next (&gsi);
> + stmt = gsi_stmt (gsi);
> + }
> + if (!stmt)
> + continue;
That would be
gsi = gsi_after_labels (bb);
If you want to ignore empty basic blocks, then followed by
if (gsi_end_p (gsi))
continue;
> + f = gimple_build_call (builtin_decl_implicit (BUILT_IN_SANCOV_TRACE),
> 0);
Generally, the text after BUILT_IN_ should be uppercase variant of the
function name, not a shorthand for it.
So I'd expect BUILT_IN_SANITIZER_COV_TRACE_PC instead.
If that makes the line too long, just use some temporary variable
say for builtin_decl_implicit result.
But I guess the most important question is, where is the
__sanitizer_cov_trace_pc function defined, I don't see it in libsanitizer
right now, is it meant for kernel only, something else?
If it is going to be in some libsanitizer library (which one), then one also
needs code to ensure that the library is linked in if -fsanitize-coverage
is used during linking. Why is it not -fsanitize=coverage, btw?
Is it incompatible with some other sanitizers, or compatible with all of
them?
Jakub
