On Wed, Dec 2, 2015 at 11:51 AM, Jakub Jelinek <ja...@redhat.com> wrote:
> 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.
>
Probably I should answer these:
> 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?
For the kernel, __sanitizer_cov_trace_pc will have to be in the kernel
itself -- we do not use any of our user-space run-times for the
kernel.
For the user space we may add it to
sanitizer_common/sanitizer_coverage_libcdep.cc,
however for userspace the benefit of having __sanitizer_cov_trace_pc
is not so big because we already have __sanitizer_cov_trace_basic_block.
__sanitizer_cov_trace_basic_block has similar functionality but a bit
more hairy implementation in the compiler
and requires an initialization step done at module init, which in the
kernel we don't have.
(Dmitry, correct me if I am wrong).
> If it is going to be in some libsanitizer library (which one),
All of them, the coverage code is in sanitizer_common
> 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?
Partially for historical reasons, partially because Clang's
-fsanitize-coverage has many subflags
(today: func, bb, edge, indir-call, 8bit-counters, trace-cmp,
trace-bb) and having them as
-fsanitize=coverage-foo,coverage-bar,coverage-zoo would be more
verbose.
--kcc
>
> Jakub
