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

[Sebastian Huber <sebastian.huber@embedded-brains.de>]

Hallo Martin,

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.

>>       }
>>     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:

/* 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?

> 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.

> 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.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/