Re: Adding new feature - Kcov

[Siddharth Muralee <siddharth.muralee%gmail.com@localhost>]

Hello, I have attached a modified patch for kcov(4). I have modified
it to be a per-process lookup instead of the earlier per-unit lookup.

It seems to working fine. I have tested it with a couple of system
calls. There is however a lot of unnecessary output coming during a
simple system call. I have attached below the output of kcov(4) for
the system call `read(-1, NULL, 0)`. I would also like to get some
input on how to reduce the noise if possible.

On Fri, 14 Dec 2018 at 12:36, Siddharth Muralee
<siddharth.muralee%gmail.com@localhost> wrote:
>
> Thanks for helping me out. Let me look at the changes you have made and I will continue working on improving it. I will also test it and get back as soon as possible.
>
> On Fri, Dec 14, 2018, 1:15 AM Maxime Villard <max%m00nbsd.net@localhost> wrote:
>>
>> Here is a modified version [1]. Compile-tested only, but it gives an idea.
>>
>> I realized that there was a problem in your patch: you give a kcov descriptor
>> on a per-process basis, but if a proc has several threads you are mixing the
>> coverage of each thread in the output buffer. This is not desired, so I
>> changed the code to be per-LWP.
>>
>> Also the lookup in _trace_pc() was bad for two reasons: it is slow, and it
>> is racy. The thing is, adding a mutex here is not allowed. So I dropped the
>> lookup and made a per-LWP pointer. As a result of that, it unfortunately
>> becomes more complicated to free the associated kcov descriptor, typically
>> in corner cases such as:
>>
>>   - A thread closes the file descriptor in the process, but another thread
>>     was being traced; in that case we ask the traced thread to free the kcov
>>     descriptor when it terminates
>>
>>   - A traced thread is killed; in that case it should disable the kcov
>>     descriptor but not free it, and this descriptor will get subsequently
>>     freed when the process terminates
>>
>> Another concern (that I didn't address) is the fact that you use device
>> units. It would seem better to me if it was not per-unit but per-process