This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]