Re: [PATCH] KCOV: Introduced tracing unique covered PCs

From: Dmitry Vyukov
Date: Tue Mar 16 2021 - 02:37:38 EST


On Mon, Mar 15, 2021 at 10:43 PM Alexander Lochmann
<info@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On 15.03.21 09:02, Dmitry Vyukov wrote:
> >>>> static notrace unsigned long canonicalize_ip(unsigned long ip)
> >>>> @@ -191,18 +192,26 @@ void notrace __sanitizer_cov_trace_pc(void)
> >>>> struct task_struct *t;
> >>>> unsigned long *area;
> >>>> unsigned long ip = canonicalize_ip(_RET_IP_);
> >>>> - unsigned long pos;
> >>>> + unsigned long pos, idx;
> >>>>
> >>>> t = current;
> >>>> - if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
> >>>> + if (!check_kcov_mode(KCOV_MODE_TRACE_PC | KCOV_MODE_UNIQUE_PC, t))
> >>>> return;
> >>>>
> >>>> area = t->kcov_area;
> >>>> - /* The first 64-bit word is the number of subsequent PCs. */
> >>>> - pos = READ_ONCE(area[0]) + 1;
> >>>> - if (likely(pos < t->kcov_size)) {
> >>>> - area[pos] = ip;
> >>>> - WRITE_ONCE(area[0], pos);
> >>>> + if (likely(t->kcov_mode == KCOV_MODE_TRACE_PC)) {
> >>>
> >>> Does this introduce an additional real of t->kcov_mode?
> >>> If yes, please reuse the value read in check_kcov_mode.
> >> Okay. How do I get that value from check_kcov_mode() to the caller?
> >> Shall I add an additional parameter to check_kcov_mode()?
> >
> > Yes, I would try to add an additional pointer parameter for mode. I
> > think after inlining the compiler should be able to regestrize it.
> >
> Should kcov->mode be written directly to that ptr?
> Otherwise, it must be written to the already present variable mode, and
> than copied to the ptr (if not NULL).

I would expect that after inlining it won't make difference in
generated code. Is so, both options are fine. Whatever leads to a
cleaner code.