RFC: Automatically test IFUNC implementations

[Andreas Jaeger]

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?

Besides that I'm fine - but like to have others chime in with their review,

thanks
Andreas
-- 
  Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
   SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
    GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
     GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126