Re: [RFC 2.0] Implementing hwcap2

["Ryan S. Arnold" <ryan dot arnold at gmail dot com>]

On Thu, Mar 28, 2013 at 2:16 PM, Roland McGrath <roland@hack.frob.com> wrote:
>> I'm presuming that the kernel will assign '26' as AT_HWCAP2.  This may
>> change when the feature is actually added.
>
> When the exact bits are predicated on kernel changes, we don't commit
> changes presuming such bit values to libc until the changes are in the
> primary kernel tree (and preferably an -rc tag so it's that much more
> likely the kernel folks won't change their minds before release).

Right, my intention is to wait until kernel support is in place before
proposing to commit the patch (in any form).

>> There isn't a usage of hwcap2 in this patch.  There will be a later
>> patch which adds some feature definitions to powerpc/hwcap.h that
>> utilize hwcap2.
>
> It's fine to separate the change that way.

Ack.

>> I'm not sure how useful tst-getauxval is until there is something it can
>> return from HWCAP2 that's not in HWCAP.  This would only be relevant to
>> architectures which have features defined in HWCAP2... which would
>> presumably only be PowerPC to start with.
>
> It's hard to see how it's useful at all in its present form.
> It tests that getauxval doesn't crash or hang.  Do we really
> care to have a test that doesn't do any correctness checking?

Right,  I actually did use it in testing to verify that my underlying
code was actually doing something but it's not useful from a
regression testing perspective.  I can't think of anything useful to
check at this point so I'll just remove it.

>> --- a/elf/dl-cache.c
>> +++ b/elf/dl-cache.c
>> @@ -22,7 +22,7 @@
>>  #include <sys/mman.h>
>>  #include <dl-cache.h>
>>  #include <dl-procinfo.h>
>> -
>> +#include <stdint.h>
>>  #include <_itoa.h>
>>
>>  #ifndef _DL_PLATFORMS_COUNT
>
> This and the other files where this was the only change seem wrong to me.
>
> If the file itself uses the name uint64_t and doesn't already include some
> header that's specified to define it, then the file is already wrong and
> the clean-up should be a separate (earlier) change from this one.  If the
> file itself doesn't use that name, then it shouldn't need the header.

I believe I identified these through warnings.. I'll verify and send a
correctness patch if that's the case.

> ldsodefs.h should include <stdint.h> since it uses uint64_t in its
> declarations.  That being the case, nothing that just uses dl_hwcap
> (even if it uses the uint64_t name along with it) should need its own
> <stdint.h> include just because of that.

Ok.

>> @@ -211,8 +212,12 @@ _dl_aux_init (ElfW(auxv_t) *av)
>>       GL(dl_phnum) = av->a_un.a_val;
>>       break;
>>        case AT_HWCAP:
>> -     GLRO(dl_hwcap) = (unsigned long int) av->a_un.a_val;
>> +     GLRO(dl_hwcap) |= (unsigned long int) av->a_un.a_val;
>>       break;
>> +      case AT_HWCAP2:
>> +     GLRO(dl_hwcap) |= ((uint64_t) av->a_un.a_val) << 32;
>> +     break;
>
> This presupposes that dl_hwcap starts out zero.  That is probably already
> the case, but the presumption is new and merits a comment.

The issue that you pointed out to me in my previous patch RFC was that
we can't presume which order AT_HWCAP and AT_HWCAP2 will be
encountered.  So I guess we have to presume that it starts out zero.
I'll add a comment.

Thanks!

Ryan