linux-kernel - Re: [PATCH] tools include: define READ_ONCE/WRITE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <5a410206-69cc-50aa-049d-1b4af8c7ed58@cn.fujitsu.com>
Date:   Tue, 28 Nov 2017 22:32:42 +0800
From:   Li Zhijian <lizhijian@...fujitsu.com>
To:     Mark Rutland <mark.rutland@....com>
CC:     <acme@...hat.com>, <linux-kernel@...r.kernel.org>,
        <philip.li@...el.com>, <paulmck@...ux.vnet.ibm.com>,
        <mingo@...nel.org>
Subject: Re: [PATCH] tools include: define READ_ONCE/WRITE_ONCE for C++


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@...well-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@...fujitsu.com>
>> ---
>>   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
>>
>