Re: [PATCH rsa/hwcap2_v3] Implement AT_HWCAP2 version 3

[Roland McGrath <roland at hack dot frob dot com>]

The details of the change seem OK except for some formatting trivia below.

My main concern with this is that it doesn't seem to make any sense
for configurations where auxv entry values are 64 bits already.
Please explain what it's all supposed to mean in that case.

> 	* elf/elf.h [AT_HWCAP2]: Add a new a_type entry.

Parens, not brackets.  Brackets are for #if conditions.

> 	* sysdeps/s390/dl-procinfo.h (_dl_procinfo): Add 'type' parameter for

A parameter name in a log entry or comment is described in caps and
without quotes.

> +      case AT_HWCAP2:
> +	hwcap |= ((uint64_t) av->a_un.a_val) << 32;

Parens are superfluous here and we tend to omit them in this case.
(All the binary operator precedence rules may be hard to remember, but
everybody can remember that unary operators have higher precedence
than binary operators.)

> -	  /* This is handled special.  */
> -	  if (_dl_procinfo (av->a_un.a_val) == 0)
> +	  /* HWCAP bits are translated into representative strings, per
> +	     platform.  */
> +	  if (_dl_procinfo (av->a_type, av->a_un.a_val) == 0)
> +
>  	    continue;

Spurious blank line here.

>  
> diff --git a/elf/elf.h b/elf/elf.h
> index 4ad4f39..b473a0e 100644
> --- a/elf/elf.h
> +++ b/elf/elf.h
> @@ -1015,6 +1015,9 @@ typedef struct
>  
>  #define AT_RANDOM	25		/* Address of 16 random bytes.  */
>  
> +#define AT_HWCAP2	26		/* Extended machine dependent hints
> +					   about processor capabilities.  */

"machine-dependent", and it wouldn't hurt to fix the AT_HWCAP comment too.
I'd say "more" rather than "extended", but that's just me.

> diff --git a/ports/sysdeps/powerpc/dl-procinfo.h b/ports/sysdeps/powerpc/dl-procinfo.h
> index b45465c..a9d98d3 100644
> --- a/ports/sysdeps/powerpc/dl-procinfo.h
> +++ b/ports/sysdeps/powerpc/dl-procinfo.h
> @@ -19,6 +19,7 @@
>  #ifndef _DL_PROCINFO_H
>  #define _DL_PROCINFO_H 1
>  
> +#include <stdint.h>
>  #include <ldsodefs.h>

You aren't adding any use of uint64_t or whatnot here, so don't add
the header.  Even if you were, this isn't needed since ldsodefs.h
already defines things of type uint64_t.

> -_dl_procinfo (int word)
> +_dl_procinfo (unsigned int type, int word)

Shouldn't the type of WORD be uint64_t, or at least an unsigned type?

> +  switch(type)

Missing space.

> +      count = MIN(_DL_HWCAP_COUNT,_DL_HWCAP2_FIRST);

Two missing spaces.

> +  for (int i = first; i < count; ++i)
>      if (word & (1 << i))

This will need to be 1ULL (or UINT64_C (1)) if WORD's type is changed.


Thanks,
Roland