Re: RFC: Automatically test IFUNC implementations

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Sep 27, 2012 at 11:38 AM, Andreas Jaeger <aj@suse.com> wrote:
> On 09/27/2012 04:27 PM, H.J. Lu wrote:
>>
>> On Thu, Sep 27, 2012 at 4:31 AM, Andreas Jaeger <aj@suse.com> wrote:
>>>
>>> On Wednesday, September 26, 2012 19:55:58 H.J. Lu wrote:
>>>>
>>>> On Wed, Sep 26, 2012 at 10:43 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>
>>>>> On Wed, Sep 26, 2012 at 10:17 AM, Richard Henderson
>>>
>>> <rth@twiddle.net> wrote:
>>>>>>
>>>>>> On 09/26/2012 10:11 AM, H.J. Lu wrote:
>>>>>>>
>>>>>>> It is hard to avoid malloc since __libc_func is called from
>>>>>>> test_init. I will first convert it to this.
>>>>>>
>>>>>>
>>>>>> Statically allocated array (or even statically sized malloc) in
>>>>>> test_init? If __libc_func indicates overflow, print an error?  It
>>>>>> should be fairly easy to set this at, say, 32 and have that
>>>>>> suffice for all calls.>
>>>>>
>>>>> This should work.  I will update hjl/ifunc/test branch in the next
>>>>> few days.
>>>>>
>>>>> Other feedbacks?
>>>>
>>>>
>>>> I updated hjl/ifunc/test branch to avoid relocations to return IFUNC
>>>> list. I am enclosing the framework and i686/x86-64 patches here.
>>>
>>>
>>> Does this pass the testsuite? I'm interested also in the abi-tests.
>>
>>
>> It passes "make check" and "make xcheck" on x32, x86-64 and ia32,
>> with and without multi-arch.  __libc_func is added to GLIBC_PRIVATE
>> and doesn't affect public ABI.
>>
>>> Looking at sysdeps/x86_64/multiarch/libc-func.c:
>>>
>>>> +static struct libc_func_test memmove_list[4];
>>>
>>>
>>> Why not have MAX_MEMMOVE instead of 4 here?
>>>>
>>>> +static struct libc_func_test *
>>>> +find_memmove (void)
>>>> +{
>>>> +  size_t i;
>>>
>>>
>>> I would assign i here:
>>>     i=0;
>>>>
>>>> +  if (HAS_SSSE3)
>>>> +    {
>>>> +      memmove_list[0] = LIBC_FUNC_INIT (__memmove_ssse3_back);
>>>> +      memmove_list[1] = LIBC_FUNC_INIT (__memmove_ssse3);
>>>> +      i = 2;
>>>> +    }
>>>> +  else
>>>> +    i = 0;
>>>
>>>
>>>
>>> And remove the above two lines, let the compiler optimize it.
>>>
>>> Or even make i=-1 initially and increase i every time.
>>>
>>> Also, I would add an assert so that we remember to increase the size of
>>> the array:
>>> assert (i < MAX_MEMMOVE);
>>>
>>>> +  memmove_list[i] = LIBC_FUNC_INIT (__memmove_sse2);
>>>
>>>
>>
>> Changed on  hjl/ifunc/test branch.  Here are updated i686/x86-64
>> patches.
>>
>> Thanks.
>
>
> in the x86-64 libc_func I see:
>
>> +# define MAX_MEMSET            2
>> +static struct libc_func_test memset_list[MAX_MEMSET + 1];
>
>
> So, you need an extra element that is NULL? I suggest you document this.
> What's the best way to add comments here? Comment the first function or
> comment above the first function a bit on what's going on?

How about this?

/* Return the array, terminated by a struct of 2 NULLs, of IFUNC
   implementations for function NAME supported on target machine.
   Return NULL if no IFUNC implementation is available.  */
extern struct libc_func_test *__libc_func (const char *name);

I will add it to  __libc_func prototype and all implementations.


---
H.J.