Re: [PATCH] tools include: define READ_ONCE/WRITE_ONCE for C++

[Li Zhijian]

On 11/28/2017 07:30 PM, Mark Rutland wrote:
> Hi,
>
> [Fixed acme's address and added more Ccs]
thanks


> On Tue, Nov 28, 2017 at 02:13:42PM +0800, Li Zhijian wrote:
> > since 6aa7de0, we failed to build perf with C++(clang)
> > to fix the following compile errors
> Sorry about this.
>
> With llvm-config and libclang from the LLVM 5.0.0 release, I can
> reproduce this.
>
> > --------------
> > lizhijian@haswell-OptiPlex-9020:~/lkp/linux/tools/perf$ make LLVM_CONFIG=/usr/bin/llvm-config-3.9 LIBCLANGLLVM=1
> > ...
> >    CC       util/probe-finder.o
> > In file included from /home/lizj/linux/tools/perf/util/util.h:13:0,
> >                   from /home/lizj/linux/tools/perf/util/util-cxx.h:20,
> >                   from util/c++/clang-c.h:5,
> >                   from util/c++/clang-test.cpp:2:
> > /home/lizj/linux/tools/include/asm/../../arch/x86/include/asm/atomic.h: In function âint atomic_read(const atomic_t*)â:
> > /home/lizj/linux/tools/include/linux/compiler.h:157:45: error: use of deleted function âatomic_read(const atomic_t*)::<anonymous union>::<constructor>()â
> >    ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })
> >                                               ^
> > /home/lizj/linux/tools/include/asm/../../arch/x86/include/asm/atomic.h:28:9: note: in expansion of macro âREAD_ONCEâ
> >    return READ_ONCE((v)->counter);
> >           ^
> > /home/lizj/linux/tools/include/linux/compiler.h:157:11: note: âatomic_read(const atomic_t*)::<anonymous union>::<constructor>()â is implicitly deleted because the default definition would be ill-formed:
> >    ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })
> >             ^
> > /home/lizj/linux/tools/include/asm/../../arch/x86/include/asm/atomic.h:28:9: note: in expansion of macro âREAD_ONCEâ
> >    return READ_ONCE((v)->counter);
> >           ^
> > /home/lizj/linux/tools/include/linux/compiler.h:157:11: error: uninitialized const member in âunion atomic_read(const atomic_t*)::<anonymous>â
> >    ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })
> >             ^
> > /home/lizj/linux/tools/include/asm/../../arch/x86/include/asm/atomic.h:28:9: note: in expansion of macro âREAD_ONCEâ
> >    return READ_ONCE((v)->counter);
> >           ^
> > /home/lizj/linux/tools/include/linux/compiler.h:157:23: note: âconst int atomic_read(const atomic_t*)::<anonymous union>::__valâ should be initialized
> >    ({ union { typeof(x) __val; char __c[1]; } __u; __read_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })
> >                         ^
> > /home/lizj/linux/tools/include/asm/../../arch/x86/include/asm/atomic.h:28:9: note: in expansion of macro âREAD_ONCEâ
> >    return READ_ONCE((v)->counter);
> >           ^
> >    LD       tests/perf-in.o
> So IIUC, g++ doesn't like the union aliasing trick that we use here, and
> doesn't believe that __val has been initialised.
>
> I haven't found a way around that yet. Ideally we'd just have a
> non-const temporary (and a __builtin_memcpy) rather than a union, but I
> can't find a way to strip const from typeof() prior to c++11.
>
> > ...
> > --------------
> >
> > Signed-off-by: Li Zhijian <lizhijian@xxxxxxxxxxxxxx>
> > ---
> >   tools/include/linux/compiler.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
> > index 07fd03c..e4c8a0d 100644
> > --- a/tools/include/linux/compiler.h
> > +++ b/tools/include/linux/compiler.h
> > @@ -86,6 +86,10 @@
> >   
> >   #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> >   
> > +#ifdef __cplusplus
> > +#define READ_ONCE(x) ACCESS_ONCE(x)
> > +#define WRITE_ONCE(x, val) ACCESS_ONCE(x) = val
> > +#else
> >   #include <linux/types.h>
> This is somewhat unfortunate, since we want to get rid of ACCESS_ONCE().

Got it


> I'll see if I can come up with something to make the existing
> {READ,WRITE}_ONCE() work, so that we can avoid issues with non-scalar
> types.

That's great if we can bring up the existing code.


Thanks
Zhijian

> Otherwise, could we please make these:
>
> #define READ_ONCE(x) 	(*(volatile typeof(x) *)&(x))
> #define WRITE_ONCE(x)	(*(volatile typeof(x) *)&(x)) = (val)
>
> Thanks,
> Mark.
>
> >   /*
> > @@ -160,6 +164,7 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
> >   #define WRITE_ONCE(x, val) \
> >   	({ union { typeof(x) __val; char __c[1]; } __u = { .__val = (val) }; __write_once_size(&(x), __u.__c, sizeof(x)); __u.__val; })
> >   
> > +#endif /* __cplusplus */
> >   
> >   #ifndef __fallthrough
> >   # define __fallthrough
> > --
> > 2.7.4