Re: arch/x86/include/asm/processor.h:698:16: sparse: sparse: incorrect type in initializer (different address spaces)

[Uros Bizjak <ubizjak@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3412 bytes --]

On Wed, Apr 3, 2024 at 7:57 PM Nathan Chancellor <nathan@kernel.org> wrote:

> > > > With GCC, you can use __typeof_unqual__ (please note underscores)
> > > > without -std=c2x [1]:
> > > >
> > > > "... Alternate spelling __typeof_unqual__ is available in all C modes
> > > > and provides non-atomic unqualified version of what __typeof__
> > > > operator returns..."
> > > >
> > > > Please also see the example in my last post. It can be compiled without -std=...
> > >
> > > With gcc >= 14. Not so with clang...
> >
> > Please note that clang-17.0.6 currently fails to compile kernel with
> > named address spaces [1]. So perhaps kernel can use __typeof_unqual__
> > (available without -std=c2x) in the hope that clang implements
> > __typeof_unqual__ in one of its next releases, following the examples
> > of GCC [2] and MSVC[3].
>
> This is now supported in clang 19.0.0 (main):
>
> https://github.com/llvm/llvm-project/commit/cc308f60d41744b5920ec2e2e5b25e1273c8704b
>
> I have inquired about applying this to the 18.x series, such that it
> would either make 18.1.3 or 18.1.4, but that is still open for
> discussion.
>
> I think the error that I mentioned at [1] is resolved with using
> __typeof_unqual__, I tested this diff, which is likely incorrect but
> allows me to continue testing without that warning/error due to -Werror:
>
> diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
> index 20696df5d567..fc77c99d2e80 100644
> --- a/arch/x86/include/asm/percpu.h
> +++ b/arch/x86/include/asm/percpu.h
> @@ -95,7 +95,7 @@
>
>  #endif /* CONFIG_SMP */
>
> -#define __my_cpu_type(var)     typeof(var) __percpu_seg_override
> +#define __my_cpu_type(var)     __typeof_unqual__(var) __percpu_seg_override
>  #define __my_cpu_ptr(ptr)      (__my_cpu_type(*ptr)*)(__force uintptr_t)(ptr)
>  #define __my_cpu_var(var)      (*__my_cpu_ptr(&(var)))
>  #define __percpu_arg(x)                __percpu_prefix "%" #x

IMO, the above change is not correct. Currently, the percpu variables
still live in generic address space, and the above casts are used at
the usage site of the percpu variable to convert pointer from generic
to disjoint __seg_gs address space (please see [2], section 6.17.5).
The -Wduplicate-decl-specifier warning at [1] (if correct) perhaps
points to the percpu accessor chain. GCC does not care about duplicate
__seg_gs conversions (the operation is idempotent), but the issue
should be corrected nevertheless.

BTW: With the above approach we get all the benefits of named address
spaces, but *not* checks for invalid access between disjoint address
spaces. This check is currently done by sparse (this is the reason for
__force in the above cast chain), but the check is not enabled by
default. The proposed improvement would *define* the percpu variable
in __seg_gs named address space, so the compiler will error out with
"assignment/return from pointer to non-enclosed address space" when
invalid access is detected (please see attached testcase, should be
compiled with gcc-14 due to usage of __typeof_unqual__) or warn with
"cast to generic address space pointer from disjoint ‘__seg_gs’
address space pointer" with explicit cast.

[1] https://lore.kernel.org/lkml/20240320173758.GA3017166@dev-arch.thelio-3990X/
[2] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html

Uros.

[-- Attachment #2: seg.c --]
[-- Type: text/x-csrc, Size: 549 bytes --]

int __seg_gs var;

void foo1 (void)
{
  asm volatile ("# %0" :: "m" (var));
}

int foo2 (void)
{
  return var;
}

int __seg_gs *bar1 (void)
{
  return &var;
}

int *bar2 (void)
{
  // return &var; /* error */
  return (int *)&var;
}

int *bar3 (void)
{
  int *p;

  //  p = &var; /* error */
  p = (int *)&var;

  return p;
}

int __seg_gs *baz1 (void)
{
  typeof(var) *p; /* (__seg_gs int *) */

  p = &var;

  return p;
}

int *baz2 (void)
{
  __typeof_unqual__(var) *p; /* (int *) */

  // p = &var; /* error */
  p = (int *)&var;

  return p;
}