Re: [PATCH] x86: Install <bits/platform/x86.h> [BZ #27958]

[Carlos O'Donell <carlos@redhat.com>]

On 6/5/21 9:59 AM, H.J. Lu via Libc-alpha wrote:
> Install <bits/platform/x86.h> for <sys/platform/x86.h> which includes
> <bits/platform/x86.h>.
> 
> Fixes BZ #27958.

The constants in bits/platform/x86.h are largely ABI given the behaviour
of the cpuid instruction. Likewise we do a consistent mapping between
the cpuid_array <-> usable_array without exposing internal details.

The API in sys/platform/x86.h has already been reviewed, discussed, and
exposes HAS_CPU_FEATURE(name) and CPU_FEATURE_USABLE(name).

Given that we get one more chance at review let me ask a few final questions.

(1) API prefixes in macros help developers remember names.

  Consistent prefix for APIs help developers remember.

  We use HAS_* but also CPU_* which requires the programmer remember
  two distinct naming strategies.

  Suggestion: CPU_FEATURE_PRESENT(), CPU_FEATURE_USABLE()?

  Note: We do this in the underlying name e.g. x86_cpu_*
        has_feature (could be is_present) vs. is_usable.

(2) ABI testing?

  - How are we making sure we don't accidentally break ABI?

    - Do we need any further testing?

  - Do we have a decoupled test to ensure a refactor doesn't break
    things?

    - We have tst-cpu-features-cpuinfo.c, which should cover
      comparison to the decoupled cpuinfo.

Notes:

- We will not be able to avoid in-place-update failures, in that rpm
  will do an atomic rename that unlinks the old libc.so.6 with the
  new libc.so.6 and if ld.so is not yet updated or updated first then
  a process that starts will crash. This makes it error prone to update
  the ABI in downstream minor updates.

> ---
>  sysdeps/x86/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
> index 346ec491b3..567ea54243 100644
> --- a/sysdeps/x86/Makefile
> +++ b/sysdeps/x86/Makefile
> @@ -5,7 +5,7 @@ endif
>  ifeq ($(subdir),elf)
>  sysdep_routines += get-cpuid-feature-leaf
>  sysdep-dl-routines += dl-get-cpu-features
> -sysdep_headers += sys/platform/x86.h
> +sysdep_headers += sys/platform/x86.h bits/platform/x86.h
>  
>  CFLAGS-get-cpuid-feature-leaf.o += $(no-stack-protector)
  


-- 
Cheers,
Carlos.