Re: [PATCH v5 01/22] elf: Avoid unnecessary slowdown from profiling with audit (BZ#15533)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 10/11/2021 09:11, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/elf/dl-reloc.c b/elf/dl-reloc.c
>> index 0d5b727c64..27f792b7b2 100644
>> --- a/elf/dl-reloc.c
>> +++ b/elf/dl-reloc.c
>> @@ -203,11 +203,25 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>    const char *errstring = NULL;
>>    int lazy = reloc_mode & RTLD_LAZY;
>>    int skip_ifunc = reloc_mode & __RTLD_NOIFUNC;
>> +  bool consider_symbind = false;
> 
> This needs __attribute__ ((unused)) or some fairly complex preprocessor
> conditional.  Otherwise, the profiling builds performed by
> build-many-glibcs.py fail.

Right, I rarely test with profiling enabled. I will check this out.

> 
>>  #ifdef SHARED
>>    /* If we are auditing, install the same handlers we need for profiling.  */
>>    if ((reloc_mode & __RTLD_AUDIT) == 0)
>> -    consider_profiling |= GLRO(dl_audit) != NULL;
>> +    {
>> +      struct audit_ifaces *afct = GLRO(dl_audit);
>> +      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>> +	{
>> +	  /* Profiling is needed only if PLT hooks are provided.  */
>> +	  if (afct->ARCH_LA_PLTENTER != NULL
>> +	      || afct->ARCH_LA_PLTEXIT != NULL)
>> +	    consider_profiling = 1;
>> +	  if (afct->symbind != NULL)
>> +	    consider_symbind = true;
>> +
>> +	  afct = afct->next;
>> +	}
>> +    }
>>  #elif defined PROF
>>    /* Never use dynamic linker profiling for gprof profiling code.  */
>>  # define consider_profiling 0
>> @@ -272,7 +286,7 @@ _dl_relocate_object (struct link_map *l, struct r_scope_elem *scope[],
>>      ELF_DYNAMIC_RELOCATE (l, scope, lazy, consider_profiling, skip_ifunc);
>>  
>>  #ifndef PROF
>> -    if (__glibc_unlikely (consider_profiling)
>> +    if (consider_profiling | consider_symbind
>>  	&& l->l_info[DT_PLTRELSZ] != NULL)
>>        {
>>  	/* Allocate the array which will contain the already found
>> diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c
>> index 61c260ddb8..b0c788eb22 100644
>> --- a/elf/dl-runtime.c
>> +++ b/elf/dl-runtime.c
>> @@ -44,6 +44,84 @@
>>  # define ARCH_FIXUP_ATTRIBUTE
>>  #endif
>>  
>> +#ifdef SHARED
>> +static void
>> +_dl_audit_symbind (struct link_map *l, struct reloc_result *reloc_result,
>> +		   const ElfW(Sym) *defsym, DL_FIXUP_VALUE_TYPE *value,
>> +		   lookup_t result)
>> +{
> 
> Maybe start out with moving this function to elf/dl-audit.c (like it
> happens later)?  It will make it easier to review the changes.

OK, I will refactor the patchset to make the elf/dl-audit.c ones first.

> 
> What are the actual function changes for _dl_audit_symbind?  The diff
> isn't very readable even with diff -uw.

There is none in fact, I move to its own function because it is not used
in two places.