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


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


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