Re: [PATCH v2] gcov: Add __gcov_info_to_gdca()

["Martin Liška" <mliska@suse.cz>]

On 7/23/21 8:10 AM, Sebastian Huber wrote:
> Hallo Martin,

Hello.

> 
> thanks for your review.
> 
> On 23/07/2021 07:31, Martin Liška wrote:
>> On 7/13/21 10:15 PM, Sebastian Huber wrote:
>>
>> Hello.
>>
>> Thanks for working on that, there's my review:
>>
>>> Add __gcov_info_to_gcda() to libgcov to get the gcda data for a gcda info in a
>>> freestanding environment.  It is intended to be used with the
>>> -fprofile-info-section option.  A crude test program which doesn't use a linker
>>> script is (use "gcc -coverage -fprofile-info-section -lgcc test.c" to compile
>>> it):
> [...]>> @@ -365,46 +426,48 @@ write_topn_counters (const struct gcov_ctr_info
>>> *ci_ptr,
>>>     if (list_sizes == NULL || counters > list_size_length)
>>>       {
>>>         list_size_length = MAX (LIST_SIZE_MIN_LENGTH, 2 * counters);
>>> -#if HAVE_SYS_MMAN_H
>>> +#if !defined(inhibit_libc) && HAVE_SYS_MMAN_H
>>>         list_sizes
>>>       = (unsigned *)malloc_mmap (list_size_length * sizeof (unsigned));
>>>   #endif
>>>         /* Malloc fallback.  */
>>>         if (list_sizes == NULL)
>>> -    list_sizes = (unsigned *)xmalloc (list_size_length * sizeof (unsigned));
>>> +    list_sizes =
>>> +      (unsigned *)(*allocate) (list_size_length * sizeof (unsigned), arg);
>>>       }
>>> -  memset (list_sizes, 0, counters * sizeof (unsigned));
>>
>> Are you sure we don't need this zeroing?
> 
> The list_sizes array is iterated two times using the same "counters" limit. In the first iteration ...
> 
>>
>>>     unsigned pair_total = 0;
>>>     for (unsigned i = 0; i < counters; i++)
>>>       {
>>>         gcov_type start = ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i + 2];
>>> +      unsigned sizes = 0;
>>> +
> 
> ... the sizes is initialized to zero here and ...
> 
>>>         for (struct gcov_kvp *node = (struct gcov_kvp *)(intptr_t)start;
>>>          node != NULL; node = node->next)
>>> -    {
>>> -      ++pair_total;
>>> -      ++list_sizes[i];
>>> -    }
>>> +    ++sizes;
>>> +
>>> +      pair_total += sizes;
>>> +      list_sizes[i] = sizes;
> 
> ... written to the array here.

Oh, good then!

> 
>>>       }
>>>     unsigned disk_size = GCOV_TOPN_DISK_COUNTERS * counters + 2 * pair_total;
>>> -  gcov_write_tag_length (GCOV_TAG_FOR_COUNTER (t_ix),
>>> -             GCOV_TAG_COUNTER_LENGTH (disk_size));
>>> +  dump_unsigned (GCOV_TAG_FOR_COUNTER (t_ix), dump, arg),
>>> +  dump_unsigned (GCOV_TAG_COUNTER_LENGTH (disk_size), dump, arg);
>>>     for (unsigned i = 0; i < counters; i++)
>>>       {
>>> -      gcov_write_counter (ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i]);
>>> -      gcov_write_counter (list_sizes[i]);
>>> +      dump_counter (ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i], dump, arg);
>>> +      dump_counter (list_sizes[i], dump, arg);
>>>         gcov_type start = ci_ptr->values[GCOV_TOPN_MEM_COUNTERS * i + 2];
>>>         unsigned j = 0;
>>>         for (struct gcov_kvp *node = (struct gcov_kvp *)(intptr_t)start;
>>>          j < list_sizes[i]; node = node->next, j++)
>>>       {
>>> -      gcov_write_counter (node->value);
>>> -      gcov_write_counter (node->count);
>>> +      dump_counter (node->value, dump, arg);
>>> +      dump_counter (node->count, dump, arg);
>>>       }
>>>       }
>>>   }
> [...]
>>> +#ifdef NEED_L_GCOV_INFO_TO_GCDA
>>> +/* Convert the gcov info to a gcda data stream.  It is intended for
>>> +   free-standing environments which do not support the C library file I/O.  */
>>> +
>>> +void
>>> +__gcov_info_to_gcda (const struct gcov_info *gi_ptr,
>>> +             void (*filename) (const char *, void *),
>>
>> What about begin_finaname_fn?
>>
>>> +             void (*dump) (const void *, unsigned, void *),
>>> +             void *(*allocate) (unsigned, void *),
>>> +             void *arg)
>>> +{
>>> +  (*filename) (gi_ptr->filename, arg);
>>> +  write_one_data (gi_ptr, NULL, dump, allocate, arg);
>>> +}
>>> +#endif /* NEED_L_GCOV_INFO_TO_GCDA */
>>>
>>
>> About gcov_write_summary: it should be also dumped in order to have a complete .gcda file, right?
> 
> How can I get access to the summary information? Here it is not available:

You only need to change gcov_write_summary in gcov-io.c.

> 
> /* Information about a single object file.  */
> struct gcov_info
> {
> [...]
> #ifndef IN_GCOV_TOOL
>    const struct gcov_fn_info *const *functions; /* pointer to pointers
>                                                    to function information  */
> #else
>    struct gcov_fn_info **functions;
>    struct gcov_summary summary;
> #endif /* !IN_GCOV_TOOL */
> };
> 
> Should we dump a memset(&summary, 0, summary) summary?
> 
>> Can we remove gcov_write_counter function?
> 
> It is no longer used. What is the policy to remove functions?

We can remove it if it's unused :)

> 
>> I think it can be handy having a script that creates a .gcda file from your reference dump output,
>> what do you think?
> 
> I think the dump output from the example is not useful for real applications. In an embedded system you would probably dump the information via a serial interface. The dump could be compressed and base64 encoded with some sort of EDAC.

Sure, but a host system needs to take such a stream and reconstruct .gcda files. That's what I mean. Btw. do you use the created .gcda files only for gcov (coverage reports), or
do you use it for -fprofile-use (PGO/FDO)? Btw. the generated .gcda files should also survive gcov-dump command.

> 
>> It would be nice having a test-case that can test your approach.
> 
> The problem is that you need the linker set to get access to the gcov information. The test program of the commit message works on my Linux machine. I am not sure if it is generic enough for the test suite. Instead of printing the information we could compare it against an expected output so that we have a self-contained test program.

Yep, that would be nice.

Martin

>