On Wed, Dec 2, 2015 at 6:11 PM, Bernd Schmidt <bschm...@redhat.com> wrote:
> On 12/02/2015 05:55 PM, Dmitry Vyukov wrote:
>>
>> Can you point to some concrete coding style violations (besides
>> function comments)?
>>
>> (flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \
>> - | SANITIZE_UNDEFINED |
>> SANITIZE_NONDEFAULT)))
>> + | SANITIZE_UNDEFINED |
>> SANITIZE_NONDEFAULT) \
>> + || flag_sanitize_coverage))
>
>
> The || should line up with the other condition (i.e. the part starting with
> flag_sanitize).
>
>> +unsigned sancov_pass (function *fun)
>
>
> Split the line after the return type.
>
>> +
>> +template<bool O0>
>> +class pass_sancov : public gimple_opt_pass
>> +{
Thanks for the review!
> This seems to be a new idiom but I find it OK.
Duplicating this code would double patch size :)
Asan pass has this duplication.
FWIW I found this template trick better than duplication of a large
chunk of code
> One thing to consider would
> be whether you really need this split between O0/optimize versions, or
> whether you can find a place in the queue where to insert it
> unconditionally. Have you considered this at all or did you just follow
> asan/tsan?
I inserted the pass just before asan/tsan because it looks like the
right place for it. If we do it after asan, it will insert coverage
for all asan-emited BBs which is highly undesirable. I also think it
is a good idea to run a bunch of optimizations before coverage pass to
not emit too many coverage callbacks (but I can't say that I am very
knowledgeable in this area). FWIW clang does the same: coverage passes
run just before asan/tsan.
>> +public:
>> + static pass_data pd ()
>> + {
>> + static const pass_data data =
>
>
> I think a static data member would be better than the unnecessary pd ()
> function. This is also unlike existing practice, and I wonder how others
> think about it. IMO a fairly strong case could be made that if we're using
> C++, then this sort of thing ought to be part of the class definition.
I vary name of the pass depending on the O0 template argument (again
following asan):
O0 ? "sancov_O0" : "sancov", /* name */
If we call it "sancov" always, then I can make it just a global var
(as all other passes in gcc).
Or I can make it a static variable of the template class and move
definition of the class (as you proposed).
What would you prefer?
