Re: [PATCH] elf: Remove -fno-tree-loop-distribute-patterns usage on dl-support

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

On 09/08/22 18:51, H.J. Lu wrote:
> On Tue, Aug 9, 2022 at 12:40 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 09/08/22 16:34, H.J. Lu wrote:
>>> On Tue, Aug 9, 2022 at 12:32 PM Adhemerval Zanella Netto
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 09/08/22 16:06, H.J. Lu wrote:
>>>>> On Tue, Aug 9, 2022 at 11:48 AM Adhemerval Zanella Netto
>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 09/08/22 15:31, H.J. Lu wrote:
>>>>>>> On Tue, Aug 9, 2022 at 5:11 AM Adhemerval Zanella Netto
>>>>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 08/08/22 19:16, H.J. Lu wrote:
>>>>>>>>
>>>>>>>>>> --- a/sysdeps/x86_64/multiarch/memset-evex-unaligned-erms.S
>>>>>>>>>> +++ b/sysdeps/x86_64/multiarch/memset-evex-unaligned-erms.S
>>>>>>>>>> @@ -44,4 +44,7 @@
>>>>>>>>>>
>>>>>>>>>>  # define USE_LESS_VEC_MASK_STORE       1
>>>>>>>>>>  # include "memset-vec-unaligned-erms.S"
>>>>>>>>>> +# if !defined NO_MULTIARCH && MINIMUM_X86_ISA_LEVEL >= 4
>>>>>>>>>> +strong_alias (__memset_evex_unaligned, __memset_generic)
>>>>>>>>>> +# endif
>>>>>>>>>>  #endif
>>>>>>>>>
>>>>>>>>> This will define 2 __memset_generic for -march=x86-64-v4.
>>>>>>>>
>>>>>>>> Are you sure? Check with a x86_64 build with -march=x86-64-v4 I am seeing:
>>>>>>>>
>>>>>>>> $ readelf -Ws string/memset*.os | grep -w __memset_generic | wc -l
>>>>>>>> 1
>>>>>>>>
>>>>>>>> And it is indeed provided only by string/memset-evex-unaligned-erms.os.
>>>>>>>
>>>>>>> You are right.  Since dl-symbol-hacks.h defines memset to an alias,
>>>>>>> should it be moved to sysdes/generic and each arch can provide a
>>>>>>> suitable alias?
>>>>>>
>>>>>> The dl-symbol-hacks.h is meant to be used solely by loader code (that's
>>>>>> why I have added to elf/), but I don't have a strong preference.
>>>>>
>>>>> The default should be no alias.  Only IFUNC targets need to define a
>>>>> proper alias.
>>>>
>>>> Alright, although I added the alias on all targets to avoid adding another
>>>> hook to specify whether memset is implemented by IFUNC (to enable the
>>>> symbol redirection instead of use the default symbol name).  I am not sure
>>>> if making arch-specific will simplify the required code here.
>>>
>>> But your patch adds alias to all targets.
>>
>> Yes, because otherwise we will need something like HAVE_MEMSET_IFUNC to add:
>>
>> #if defined SHARED && HAVE_MEMSET_IFUNC
>> asm ("memset = __memset_generic");
>> #endif
>>
>> I don't have a strong preference, although it is another this will require
>> to have kernel-features.h (which we usually add the HAVE_*) on elf objects.
> 
> There are
> 
> elf/Makefile:CFLAGS-dl-tunables.c += -fno-tree-loop-distribute-patterns
> elf/Makefile:CFLAGS-rtld.c += -fno-tree-loop-distribute-patterns

Unfortunately we can't really remove these two without refactoring the code to
avoid the memset constructions because functions call are not really allowed.

For dl-tunables.c I am not sure if we actually need to filter out the invalid
options to create a new tunable, since we already do not set them for glibc
itself.  The program will see the original value, but it show not matter since
glibc would already have handle it anyway and it might avoid the need to strdup 
the GLIBC_TUNABLES.

> elf/Makefile:CFLAGS-dl-support.c = -fno-tree-loop-distribute-patterns
> include/libc-symbols.h:    __attribute__ ((__optimize__
> ("-fno-tree-loop-distribute-patterns")))

We need this to void the memset recursion when gcc see the memset implementation
as memset itself.

> string/test-string.h:    __attribute__ ((__optimize__
> ("-fno-tree-loop-distribute-patterns")))
> 
> There should be no check on SHARED.   It can be something like
> 
> #define SYMBOL_STR1(s) #s
> #define SYMBOL_STR(s) SYMBOL_STR1(s)
> 
> #ifdef DEFAULT_MEMSET
> asm ("memset = " _SYMBOL_STR (DEFAULT_MEMSET));
> #endif

Yeah, it could work.  I will redo based on this, it should simplify the changes
for other ports.