This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: PING: PATCH: Automatically test IFUNC implementations
- From: Roland McGrath <roland at hack dot frob dot com>
- To: "H.J. Lu" <hjl dot tools at gmail dot com>
- Cc: Richard Henderson <rth at twiddle dot net>, Andreas Jaeger <aj at suse dot com>, libc-alpha at sourceware dot org
- Date: Fri, 5 Oct 2012 15:47:49 -0700 (PDT)
- Subject: Re: PING: PATCH: Automatically test IFUNC implementations
- References: <CAMe9rOoktiz4=dSHSySnXQRywVRNF7Jbxrda6Hy3=TvZ4WgkFQ@mail.gmail.com>
As always, it's really easiest if the final version of the patches are
posted in the normal fashion for review.
Looking at the branch, some things that need fixing. After fixing these,
please post the patch series again in the proper way for final review.
* TYpo in log entry.
* array-size arg to __libc_func should be size_t, return value also size_t
* __libc_func is a poor name, make it something more verbose,
e.g. __libc_enumerate_implementations_for_test
* libc-func doesn't belong in string/Makefile and string/Versions.
It's a generic thing. Put it someplace like misc/ instead.
* C files don't belong in sysdeps/generic/ without special reason.
Put it in the same directory whose Makefile lists it, like all stubs are.
* There is no real need to use %ifdef in Versions.
Naming a nonexistent symbol doesn't hurt. Just drop the conditional.
* 'sizeof foo', not 'sizeof (foo)' when foo is not a type name.
* All new files need a top line that's a descriptive comment.
* __attribute_used__ is wrong for unreferenced parameters.
Use __attribute__ ((unused)) instead.
* Each find_* in libc-func.c should have a comment saying it must match the
set used in the corresponding IFUNC selector, naming the exact source
file name (sysdeps/.../foo.S) that contains the selector. Likewise, each
selector's source should have a comment with a reminder to update
sysdeps/.../libc-func.c when changing the set used by the selector.
* Why does libc-func.c need "#ifndef NOT_IN_libc"?
This file should never be compiled at all in any circumstance where
NOT_IN_libc is defined.
* Use a macro like:
#define FIND_FUNC(func) \
if (strcmp (name, #func) == 0) \
return find_##func (array, max);
rather than repeating the pattern.
* In each find_* function, use a simple pattern of:
array[i++] = LIBC_FUNC_INIT (foobar);
rather than error-prone arithmetic.
In fact, I'd use a macro like:
#define FIND_FUNC(cond, func) \
if (cond) \
array[i++] = LIBC_FUNC_INIT (func)
Then call this as:
FIND_FUNC (HAS_SSSE3, __memmove_chk_ssse3_back);
FIND_FUNC (HAS_SSSE3, __memmove_chk_ssse3);
FIND_FUNC (1, __memmove_chk_sse2);
To further avoid boilerplate, add another macro:
#define FUNC_FINDER(name, ...) \
static int \
find_##name (struct libc_func_test *array, size_t max) \
{ \
size_t i = 0; \
__VA_ARGS__; \
}
Then each one looks like:
FUNC_FINDER (__memmove_chk,
FIND_FUNC (HAS_SSSE3, __memmove_chk_ssse3_back),
FIND_FUNC (HAS_SSSE3, __memmove_chk_ssse3),
FIND_FUNC (1, __memmove_chk_sse2)
)
Thanks,
Roland