> > Hello.
> > 
> > As noticed in the PR, it's quite tricky to not run malloc (or calloc)
> > in context of libgcov. I'm suggesting a new approach where we'll first
> > use the pre-allocated static buffer in hope that malloc function is 
> > initialized
> > and so every call to calloc can happen. That's why I increased number of KVP
> > to 64 and I believe one reaches malloc pretty soon in an application run.
> > 
> > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > 
> > Ready to be installed?
> > Thanks,
> > Martin
> > 
> > gcc/ChangeLog:
> > 
> >      PR gcov-profile/97461
> >      * gcov-io.h (GCOV_PREALLOCATED_KVP): Pre-allocate 64
> >      static counters.
> > 
> > libgcc/ChangeLog:
> > 
> >      PR gcov-profile/97461
> >      * libgcov.h (gcov_counter_add): Use first static counters
> >      as it should help to have malloc wrappers set up.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> >      PR gcov-profile/97461
> >      * gcc.dg/tree-prof/pr97461.c: New test.

Looks reasonable, but I do not like very much the non-configurable
preallocation since libgcov was meant to be useful for embedded targets
and not consume too much.  I guess we could handle that incrementally,
how the llvm's option for preallocated pool size is implemented?

Honza
> > ---
> >   gcc/gcov-io.h                            |  2 +-
> >   gcc/testsuite/gcc.dg/tree-prof/pr97461.c | 58 ++++++++++++++++++++++++
> >   libgcc/libgcov.h                         | 24 +++-------
> >   3 files changed, 65 insertions(+), 19 deletions(-)
> >   create mode 100644 gcc/testsuite/gcc.dg/tree-prof/pr97461.c
> > 
> > diff --git a/gcc/gcov-io.h b/gcc/gcov-io.h
> > index 4dba01c78ce..4e95c7c82ee 100644
> > --- a/gcc/gcov-io.h
> > +++ b/gcc/gcov-io.h
> > @@ -293,7 +293,7 @@ GCOV_COUNTERS
> >   #define GCOV_TOPN_MAXIMUM_TRACKED_VALUES 32
> > 
> >   /* Number of pre-allocated gcov_kvp structures.  */
> > -#define GCOV_PREALLOCATED_KVP 16
> > +#define GCOV_PREALLOCATED_KVP 64
> > 
> >   /* Convert a counter index to a tag.  */
> >   #define GCOV_TAG_FOR_COUNTER(COUNT)                \
> > diff --git a/gcc/testsuite/gcc.dg/tree-prof/pr97461.c 
> > b/gcc/testsuite/gcc.dg/tree-prof/pr97461.c
> > new file mode 100644
> > index 00000000000..8d21a3ef421
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tree-prof/pr97461.c
> > @@ -0,0 +1,58 @@
> > +/* PR gcov-profile/97461 */
> > +/* { dg-options "-O2 -ldl" } */
> > +
> > +#define _GNU_SOURCE
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +
> > +static int malloc_depth = 0;
> > +
> > +static char memory[128* 1024];
> > +static size_t memory_p = 0;
> > +
> > +void f1(void) {}
> > +void f2(void) {}
> > +
> > +typedef void (*fun_t)(void);
> > +static const fun_t funs[2] = { f1, f2, };
> > +
> > +static void * malloc_impl(size_t size) {
> > +    void * r = &memory[memory_p];
> > +    memory_p += size;
> > +
> > +    // force TOPN profile
> > +    funs[size % 2]();
> > +    return r;
> > +}
> > +
> > +// Override default malloc, check it it get s called recursively
> > +void * malloc(size_t size) {
> > +    // Must not be called recursively. Malloc implementation does not 
> > support it.
> > +    if (malloc_depth != 0) __builtin_trap();
> > +
> > +    ++malloc_depth;
> > +      void * r = malloc_impl(size);
> > +    --malloc_depth;
> > +    return r;
> > +}
> > +
> > +// Called from gcov
> > +void *calloc(size_t nmemb, size_t size) {
> > +    // Must not be called recursively.  Malloc implementation does not 
> > support it.
> > +    if (malloc_depth != 0) __builtin_trap();
> > +
> > +    ++malloc_depth;
> > +      void * r = malloc_impl(size * nmemb);
> > +      memset(r, 0, size * nmemb);
> > +    --malloc_depth;
> > +    return r;
> > +}
> > +
> > +void free(void *ptr){}
> > +
> > +int main() {
> > +    void * p = malloc(8);
> > +    return p != 0 ? 0 : 1;
> > +}
> > diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h
> > index 8be5bebcac0..e70cf63b414 100644
> > --- a/libgcc/libgcov.h
> > +++ b/libgcc/libgcov.h
> > @@ -404,22 +404,16 @@ gcov_counter_add (gcov_type *counter, gcov_type value,
> >       *counter += value;
> >   }
> > 
> > -/* Allocate gcov_kvp from heap.  If we are recursively called, then 
> > allocate
> > -   it from a list of pre-allocated pool.  */
> > +/* Allocate gcov_kvp from statically pre-allocated pool,
> > +   or use heap otherwise.  */
> > 
> >   static inline struct gcov_kvp *
> >   allocate_gcov_kvp (void)
> >   {
> >     struct gcov_kvp *new_node = NULL;
> > 
> > -  static
> > -#if defined(HAVE_CC_TLS)
> > -__thread
> > -#endif
> > -  volatile unsigned in_recursion ATTRIBUTE_UNUSED = 0;
> > -
> >   #if !defined(IN_GCOV_TOOL) && !defined(L_gcov_merge_topn)
> > -  if (__builtin_expect (in_recursion, 0))
> > +  if (__gcov_kvp_pool_index < GCOV_PREALLOCATED_KVP)
> >       {
> >         unsigned index;
> >   #if GCOV_SUPPORTS_ATOMIC
> > @@ -430,17 +424,11 @@ __thread
> >   #endif
> >         if (index < GCOV_PREALLOCATED_KVP)
> >       new_node = &__gcov_kvp_pool[index];
> > -      else
> > -    /* Do not crash in the situation.  */
> > -    return NULL;
> >       }
> > -  else
> >   #endif
> > -    {
> > -      in_recursion = 1;
> > -      new_node = (struct gcov_kvp *)xcalloc (1, sizeof (struct gcov_kvp));
> > -      in_recursion = 0;
> > -    }
> > +
> > +  if (new_node == NULL)
> > +    new_node = (struct gcov_kvp *)xcalloc (1, sizeof (struct gcov_kvp));
> > 
> >     return new_node;
> >   }
> 

Reply via email to