On Fri, Jan 18, 2019 at 04:38:55PM +0100, Anton Lindqvist wrote:
> > - You allocate nmemb * sizeof(uintptr_t) bytes.
> > - kd_buf[nmemb - 1] is the largest valid index.
> > - kd_nmemb is nmemb - 1.
> > - kd_buf[idx * 4 + 4] is the access at the largest position.
> > - So return if (idx * 4 + 4 > kd_nmemb).
>
> You're right, I forgot about the `kd_nmemb = nmemb - 1' initialization.
> I went ahead and matched the existing style in
> __sanitizer_cov_trace_pc() but inverting the conditional could be done
> later.
As long as the syle is the same, it is fine. Then I have to think
about it only once.
> + idx = kd->kd_buf[0];
> + if (idx * 4 + 4 < kd->kd_nmemb) {
> + kd->kd_buf[idx * 4 + 1] = type;
> + kd->kd_buf[idx * 4 + 2] = arg1;
> + kd->kd_buf[idx * 4 + 3] = arg2;
> + kd->kd_buf[idx * 4 + 4] = pc;
> + kd->kd_buf[0] = idx + 1;
> + }
> +}
You changed an off by two to an off by one. Compare trace_cmp()
with __sanitizer_cov_trace_pc().
idx = kd->kd_buf[0];
if (idx < kd->kd_nmemb) {
kd->kd_buf[idx + 1] = (uintptr_t)__builtin_return_address(0);
kd->kd_buf[0] = idx + 1;
}
One of them must be wrong as the largest index accessed and the
index in the check differ. I would prefer "if (idx * 4 + 4 <=
kd->kd_nmemb)" and "if (idx + 1 <= kd->kd_nmemb)". The latter is
equivalent to the existing check.
bluhm
