On Thu, Mar 20, 2025 at 12:49 AM Uros Bizjak <ubizjak@xxxxxxxxx> wrote: > > On Thu, Mar 20, 2025 at 12:17 AM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Wed, Mar 19, 2025 at 12:44 PM Uros Bizjak <ubizjak@xxxxxxxxx> wrote: > > > > > > On Wed, Mar 19, 2025 at 7:56 PM Alexei Starovoitov > > > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > > > > > On Wed, Mar 19, 2025 at 9:06 AM Uros Bizjak <ubizjak@xxxxxxxxx> wrote: > > > > > > > > > > On Wed, Mar 19, 2025 at 3:55 PM Alexei Starovoitov > > > > > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > > > > > > > > > On Wed, Mar 19, 2025 at 7:36 AM Kumar Kartikeya Dwivedi > > > > > > <memxor@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > I've sent a fix [0], but unfortunately I was unable to reproduce the > > > > > > > > > problem with an LLVM >= 19 build, idk why. I will try with GCC >= 14 > > > > > > > > > as the patches require to confirm, but based on the error I am 99% > > > > > > > > > sure it will fix the problem. > > > > > > > > > > > > > > > > Probably because __seg_gs has CC_HAS_NAMED_AS depends on CC_IS_GCC. > > > > > > > > Let me give it a go with GCC. > > > > > > > > > > > > > > > > > > > > > > Can confirm now that this fixes it, I just did a build with GCC 14 > > > > > > > where Uros's __percpu checks kick in. > > > > > > > > > > > > Great. Thanks for checking and quick fix. > > > > > > > > > > > > btw clang supports it with __attribute__((address_space(256))), > > > > > > so CC_IS_GCC probably should be relaxed. > > > > > > > > > > https://github.com/llvm/llvm-project/issues/93449 > > > > > > > > > > needs to be fixed first. Also, the feature has to be thoroughly tested > > > > > (preferably by someone having a deep knowledge of clang) before it is > > > > > enabled by default. > > > > > > > > clang error makes sense to me. > > > > > > It is not an error, but an internal compiler error. This should never happen. > > > > Not quite. llvm backends don't have a good way to explain the error, > > but this is invalid condition. > > Arguably llvm should do a better job in such cases instead of > > printing stack trace. > > > > > > > > > What does it even mean to do addr space cast from percpu to normal address: > > > > > > > > __typeof__(int __seg_gs) const_pcpu_hot; > > > > void *__attribute____UNIQUE_ID___addressable_const_pcpu_hot612 = > > > > (void *)(long)&const_pcpu_hot; > > > > > > Please see [1] for an explanation. > > > > > > [1] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html#x86-Named-Address-Spaces > > > > You didn't answer my question. > > Actually, the above link explains and documents the issue: > > "... these address spaces are not considered to be subspaces of the > generic (flat) address space. This means that explicit casts are > required to convert pointers between these address spaces and the > generic address space. In practice the application should cast to > uintptr_t and apply the segment base offset that it installed > previously." > > IOW, for __seg_gs address space, there exists no (known) offset that > would define it as a subspace of the generic space. It is gs: prefix > that results in segment override that "switches" to __seg_gs address > space. So, to convert the pointer from __seg_gs to (nonsensical!) > generic address space, GCC allows explicit (void *)(uintptr_t) cast > that in effect just strips gs: prefix from the address. You can then > use the pointer as a pointer to a generic space, but you can't use it > to dereference data from __seg_gs address space - this would be > nonsensical, so (__seg_gs void *)(uintptr_t) cast is needed to convert > pointer back to __seg_gs AS. tbh, I don't see how the above doc sentence means "just strip gs:". But ok, if that's what gcc folks clarified as true intent and it's not going to change. btw both compilers disallow automatic variables with address space qualifier and that makes sense, but if "just strip gs:" would be the rule then auto var with gs should have meant "just strip" too. Weird. > > As suspected, gcc is producing garbage code. > > Nope, this is expected and well documented behavior. > > > See: > > https://godbolt.org/z/ozozYY3nv > > > > For > > void *ptr = (void *)(long)&pcpu_hot; > > > > gcc emits > > .quad pcpu_hot > > which is nonsensical, while clang refuses to produce garbage > > and dumps stack. > > > > Sadly, both compilers produce garbage for ret_addr() > > No, they are correct. The pointer is explicitly cast to generic > address space and this is what you get. > > > and both compilers produce correct code for ret_value(). > > At least something. > > > > Uros, > > your percpu code is broken. > > you shouldn't rely on gcc producing garbage. > > Sooner or later gcc will start erroring on it just as clang. > > It won't. It is well documented behavior, as documented in [1]. > Regarding linux code, you "should not" pass a pointer to generic > address space to dereference percpu data. Currently, > __verify_percpu_ptr() only triggers a warning when sparse checking is > used, but my patchset will now enforce this as a compile-time error > (this was a much sought feature, and it was possible to implement only > recently by using the newly introduced typeof_unqual() operator). That value proposition of the patch is clear. It's a good check, no doubt. My point that compilers could have done it just fine without using this "just strip gs:" rule for global percpu variables. I suspect it should be possible to craft the macro without assigning (void *)(long)&pcpu_hot into global var. And both compilers would have worked. > Rest > assured, before enabling this feature in linux, plenty of people > unsuccessfully tried to poke a hole in this functionality and long > threads are archived where address space functionality was discussed > to death. ;) > > BTW: You can use: > > --cut here-- > diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h > index 474d648bca9a..e6a7525c9db9 100644 > --- a/arch/x86/include/asm/percpu.h > +++ b/arch/x86/include/asm/percpu.h > @@ -105,6 +105,10 @@ > # define __my_cpu_type(var) typeof(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))) > + > +# if __has_attribute(address_space) && defined(USE_TYPEOF_UNQUAL) > +# define __percpu_qual __attribute__((address_space(3))) I see, so for undefined addr spaces clang x86 just ignores it. Weird. But ok.
