This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: PING: PATCH: Automatically test IFUNC implementations
On Tue, Oct 9, 2012 at 1:30 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> + * Rules (tests): Filter out $(tests-ifunc) if multi-arch isn't
>> + enabled.
>> + (xtests): Filter out $(xtests-ifunc) if multi-arch isn't enabled.
>> + * include/libc-func.h: New file.
>> + * misc/libc-func.c: Likewise.
>> + * misc/Makefile (routines): Add libc-func if multi-arch is
>> + enabled.
>
> Be technically precise about conditionals:
>
> * Rules [$(multi-arch) = no] (tests): Filter out $(tests-ifunc).
>
> etc.
I will make the change.
>> + * misc/Versions: Add __libc_supported_implementations to
>> + GLIBC_PRIVATE
>
> * misc/Versions (GLIBC_PRIVATE): Add __libc_supported_implementations.
I will make the change.
>> + * string/test-string.h: Include <libc-func.h>.
>> + (func_list): New. Defined only if TEST_IFUNC and TEST_NAME are
>> + defined.
>> + (func_count): Likewise.
>> + (impl_count): Likewise.
>> + (impl_array): Likewise.
>
> New what?!?! And we have a convention for conditionals. Use it!
>
> [TEST_IFUNC && TEST_NAME] (func_list, func_count): New variables.
>
> etc.
I will make the change.
>> +++ b/include/libc-func.h
>
> This is a pretty poor name given the function's renaming.
> Since this purely for IFUNC cases, lets have ifunc in the name.
> How about libc-ifunc-impl-list.h, struct libc_ifunc_impl, and
> __libc_ifunc_impl_list (ifunc-impl-list.c)?
It sounds reasonable.
>> +ifneq ($(multi-arch),no)
>> +routines += libc-func
>> +endif
>
> Let's just make it unconditional and let the trivial stub version be built
> when multiarch is not enabled.
Will do.
>> +/* Default __libc_supported_implementations.
>
> /* Enumerate available IFUNC implementations of a function. Stub version.
>
>> +/* Fill ARRAY of MAX elements with IFUNC implementations for function
>> + NAME supported on target machine and return the number of valid
>
> "on this machine"
I will change it.
> This whole scheme doesn't include any way to notice which or how many
> implementations you aren't testing because your machine is incompatbile.
>
> I'm a bit inclined to make the struct include a "bool usable" field, have
> the array always filled with all function pointers, and let the testers
> skip the ones with usable==false. Then the testers can emit output saying
> "there were N variants of foo I could not test". Eventually we can feed
> this into some more full scheme to ensure that all the variants have really
> been tested well.
I will give it a try.
>> + for (impl = __start_impls; impl < __stop_impls; ++impl) \
>> + impl_count++; \
>
> A loop is a lousy way to calculate a subtraction.
I will fix it.
>> +#if defined TEST_IFUNC && defined TEST_NAME
>> +# define SIZEOF_ARRAY(array) (sizeof array / sizeof array[0])
>> + func_count = __libc_supported_implementations (TEST_NAME, func_list,
>> + SIZEOF_ARRAY (func_list));
>
> Don't define a macro to use it only once.
I will change it.
Thanks.
--
H.J.