[PATCHv2] powerpc: ABI change - add HWCAP/HWCAP2/platform info to TCB

[Carlos Eduardo Seo]

> On Aug 26, 2015, at 5:40 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
> On 08/12/2015 03:56 PM, Carlos Eduardo Seo wrote:
>> - versioned symbol to track this change.
>> - no ‘set’ API in ppc.h
>> - no per-thread initialization
>> - added platform number (derived from AT_PLATFORM string) for future implementation of __builtin_cpu_is() in gcc.
> 
> Getting there, probably two more iterations.
> 
> You have concurrency issues.

Hm.. I didn’t get any problems like those during my tests, but I understand your point. I’ll double-check and make the necessary changes.

> 
> You have manual issues.

Got it. I’ll change that.
> 
>> diff --git a/sysdeps/powerpc/Versions b/sysdeps/powerpc/Versions
>> index 2aebf7c..c2731b3 100644
>> --- a/sysdeps/powerpc/Versions
>> +++ b/sysdeps/powerpc/Versions
>> @@ -6,6 +6,9 @@ libm {
>> }
>> 
>> libc {
>> +  GLIBC_2.23 {
>> +    init_hwcapinfo;
>> +  }
> 
> Please place GLIBC_2.23 *after* GLIBC_2.3.4.
> 
> All version files for consistency should order the version numbers
> increasingly (GLIBC_PRIVATE is always last).

OK, will do.
> 
> Test is insufficient.
> 
> Test should start multiple threads and verify that from at least 2 threads
> the values are sensible.
> 
> This would have caught the bug that I believe you have above, where only
> the main threads TCB is updated with the right hwcap/hwcap2 values.

OK, I’ll change it to multiple threads.
>> 
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/dl-support.c b/sysdeps/unix/sysv/linux/powerpc/dl-support.c
>> new file mode 100644
>> index 0000000..b221a4c
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/powerpc/dl-support.c
>> @@ -0,0 +1,25 @@
>> +/* Operating system support for dynamic linking code in static glibc.
>> +   Linux/PPC version.
>> +   Copyright (C) 2015 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library.  If not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#define DL_PLATFORM_AUXV				\
>> +	case AT_PLATFORM:				\
>> +	  GLRO(dl_platform) = (void *) av->a_un.a_val;	\
>> +	  break;
> 
> How did you test this?

The patch relies on dl_platform to write the data to the TCB. So I created a simple statically linked binary that reads the thread pointer (r13) + the offset to the platform string field and checked if it was correct.

Thanks for the review,

-- 
Carlos Eduardo Seo
Software Engineer - Linux on Power Toolchain
cseo@linux.vnet.ibm.com