Re: PATCH: Check GLIBC_IFUNC to enable/disable ifunc features

[Mike Frysinger <vapier at gentoo dot org>]

On 29 Jun 2016 11:15, H.J. Lu wrote:
> On Tue, Jun 28, 2016 at 7:28 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > should have a comment or two mentioning why you need to code all of
> > these things ad-hoc
> >
> > also needs a manual update
> 
> I am not sure if it needs manual update.  I consider it more for glibc
> developers than for normal end users.

it needs documenting somewhere beyond the commit message.  i guess we
don't have a "hacking" page somewhere, so at least the comment in the
source file will have to do.

> > seems like the feature compare is case-sensitive ?  but some of the
> > feature strings are inconsistent case ?  would be nice if they were
> > standardized (like all lower case).
> 
> I'd like to match the ones in cpu-features.h so that one can cut and
> paste from it.
> 
> Here is the updated patch.  Any more feedbacks?

you should probably note the intention to support parsing in set*id
env (i.e. not checking the secure vars).  or make it so that you skip
the logic if the secure vars are set.  you note it embedded deep in
the code, but be nicer to be explicit in the overview.

> +/* Return true if the first LEN bytes of strings A and B are the same
> +   where LEN != 0.  We don't use string/memory functions since they may

"don't" -> "can't"

> +   not be available here and this version is faster for its usage.  */

i'd insert "because they trigger an ifunc resolve loop" or something

> +   feature which isn't availble.  */

"available"

> +  while (*env != NULL)
> +    {
> +      const char *p, *end;
> +      size_t len = 13;
> +      end = *env;

GNU style says blank lines after decls
-mike

Attachment: signature.asc
Description: Digital signature