Re: [PATCH v1] arm64: allow building with kcov coverage on ARM64

[Mark Rutland]

On Tue, Apr 12, 2016 at 01:17:00PM +0200, Alexander Potapenko wrote:
> On Mon, Apr 4, 2016 at 7:30 PM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
> > I did not look at all boot crashes and hangs. The low level arch code
> > like interrupts and early bootstrap is not interesting in this
> > setting, so I just bisected down to file level and excluded it. I
> > looked at one crash, though. It was related to setup of permanent
> > per-cpu storage, the kcov callback was emitted into a critical
> > sequence of instructions that switches per-cpu storage from bootstrap
> > to the real one, and access to 'current' faulted in that callback. In
> > general, for the boot issue it's better to exclude files lazily as we
> > discover new issues.
> >
> > Besides the boot issues, other files are excluded for two reasons:
> > 1. non-deterministic coverage (like interrupts and mutex slow paths).
> > 2. excessive coverage, for example memcpy-like loop will produce O(N)
> > coverage since kcov is trace-based. I guess that delay.c falls into
> > this category.
> >
> > We don't need 100% deterministic coverage. I agree that it's not
> > feasible. User-space part of syzkaller (kcov-based fuzzer) tries to
> > work around it with some heuristics. But I've tried to to eliminate
> > some frequent and common sources of non-determinism. I've repeatedly
> > collected coverage from a simple program containing
> > mmap-open-read-close, and eliminated all frequent, large spikes of
> > coverage one by one.
> >
> > Re delay.c: on x86 it is not inlined, and some parts are written in C
> > so disable of instrumentation worked. Is it inlined on arm64? I see at
> > least the following in the c file:
> >
> > void __delay(unsigned long cycles)
> > {
> >         cycles_t start = get_cycles();
> >
> >         while ((get_cycles() - start) < cycles)
> >                 cpu_relax();
> > }
> 
> Mark,
> 
> Looks like we haven't reached the consensus on this topic yet.
> Do you have anything to comment on what Dmitry said?

I'm still concerned that we only seem to have a coarse understanding of
the issues, but I guess that cannot be helped.

I'd like to make sure that if there's anything we must inhibit the
coverage of for arm64, we have a good, documented (comment or commit
message) understanding of why. That allows us to re-evaluate the
situation as code changes.

Given we don't have much fine-grained knowledge of that sort from x86,
it looks like we have to figure that out from scratch.

As for deterministic coverage, I guess we have to see what happens and
make judgements on a case-by-case basis.

> I also wonder if we can, say, land the change to arch/arm64/Kconfig
> separately from makefile changes that improve the precision or fix
> certain build configurations.

I assume that 'precision' here means 'determinism'.

I mostly agree with that, though I would like to see the feature working
from the point it's merged. i.e. any known boot/runtime failures should
be solved now, and as above, we should somehow document why each change
is necessary.

Changes relating to determinism are a bit different, and should be
evaluated separately/subsequently. We may want to annotate those
differently, as there may be cases where non-deterministic coverage data
is useful (e.g. for something other than syzkaller).

Thanks,
Mark.