[PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[Dave Jiang]

Some gcc toolchain are optimizing out the memcpy and this causes dax-errors
to not trigger the SIG_BUS when doing memcpy on an mmap'd buffer. By moving
the buffer to a global variable this bypasses the optimization and allow
the test to work as intended.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 test/dax-errors.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/test/dax-errors.c b/test/dax-errors.c
index 11d0031..9ea5c91 100644
--- a/test/dax-errors.c
+++ b/test/dax-errors.c
@@ -17,6 +17,8 @@
 
 static sigjmp_buf sj_env;
 static int sig_count;
+/* buf is global in order to avoid gcc memcpy optimization */
+static void *buf;
 
 static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
 {
@@ -27,7 +29,7 @@ static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
 
 static int test_dax_read_err(int fd)
 {
-	void *base, *buf;
+	void *base;
 	int rc = 0;
 
 	if (fd < 0) {

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[Vishal Verma]

On 09/15, Dave Jiang wrote:
> Some gcc toolchain are optimizing out the memcpy and this causes dax-errors
> to not trigger the SIG_BUS when doing memcpy on an mmap'd buffer. By moving
> the buffer to a global variable this bypasses the optimization and allow
> the test to work as intended.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  test/dax-errors.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> diff --git a/test/dax-errors.c b/test/dax-errors.c
> index 11d0031..9ea5c91 100644
> --- a/test/dax-errors.c
> +++ b/test/dax-errors.c
> @@ -17,6 +17,8 @@
>  
>  static sigjmp_buf sj_env;
>  static int sig_count;
> +/* buf is global in order to avoid gcc memcpy optimization */
> +static void *buf;
>  
>  static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
>  {
> @@ -27,7 +29,7 @@ static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
>  
>  static int test_dax_read_err(int fd)
>  {
> -	void *base, *buf;
> +	void *base;
>  	int rc = 0;
>  
>  	if (fd < 0) {
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On
> Behalf Of Dave Jiang
> Sent: Thursday, September 15, 2016 5:28 PM
> To: vishal.l.verma@intel.com
> Cc: linux-nvdimm@lists.01.org
> Subject: [PATCH] ndctl: move test/dax-errors buffer to global to
> avoid gcc optimization
> 
> Some gcc toolchain are optimizing out the memcpy and this causes dax-
> errors
> to not trigger the SIG_BUS when doing memcpy on an mmap'd buffer. By
> moving
> the buffer to a global variable this bypasses the optimization and
> allow
> the test to work as intended.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  test/dax-errors.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/test/dax-errors.c b/test/dax-errors.c
> index 11d0031..9ea5c91 100644
> --- a/test/dax-errors.c
> +++ b/test/dax-errors.c
> @@ -17,6 +17,8 @@
> 
>  static sigjmp_buf sj_env;
>  static int sig_count;
> +/* buf is global in order to avoid gcc memcpy optimization */
> +static void *buf;
> 
>  static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
>  {
> @@ -27,7 +29,7 @@ static void sigbus_hdl(int sig, siginfo_t *siginfo,
> void *ptr)
> 
>  static int test_dax_read_err(int fd)
>  {
> -	void *base, *buf;
> +	void *base;
>  	int rc = 0;
> 
>  	if (fd < 0) {
> 

I've run into that kind of problem before, and found that
marking *buf as volatile (and leaving it inside the function)
tends to be honored better by aggressive optimizing compilers
and linkers.

---
Robert Elliott, HPE Persistent Memory



_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[Dave Jiang]

On 09/15/2016 06:18 PM, Elliott, Robert (Persistent Memory) wrote:
> 
> 
>> -----Original Message-----
>> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On
>> Behalf Of Dave Jiang
>> Sent: Thursday, September 15, 2016 5:28 PM
>> To: vishal.l.verma@intel.com
>> Cc: linux-nvdimm@lists.01.org
>> Subject: [PATCH] ndctl: move test/dax-errors buffer to global to
>> avoid gcc optimization
>>
>> Some gcc toolchain are optimizing out the memcpy and this causes dax-
>> errors
>> to not trigger the SIG_BUS when doing memcpy on an mmap'd buffer. By
>> moving
>> the buffer to a global variable this bypasses the optimization and
>> allow
>> the test to work as intended.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  test/dax-errors.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/test/dax-errors.c b/test/dax-errors.c
>> index 11d0031..9ea5c91 100644
>> --- a/test/dax-errors.c
>> +++ b/test/dax-errors.c
>> @@ -17,6 +17,8 @@
>>
>>  static sigjmp_buf sj_env;
>>  static int sig_count;
>> +/* buf is global in order to avoid gcc memcpy optimization */
>> +static void *buf;
>>
>>  static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
>>  {
>> @@ -27,7 +29,7 @@ static void sigbus_hdl(int sig, siginfo_t *siginfo,
>> void *ptr)
>>
>>  static int test_dax_read_err(int fd)
>>  {
>> -void *base, *buf;
>> +void *base;
>>  int rc = 0;
>>
>>  if (fd < 0) {
>>
> 
> I've run into that kind of problem before, and found that
> marking *buf as volatile (and leaving it inside the function)
> tends to be honored better by aggressive optimizing compilers
> and linkers.

I'll make the change.

> 
> ---
> Robert Elliott, HPE Persistent Memory
> 
> 
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[Dave Jiang]

On 09/15/2016 06:18 PM, Elliott, Robert (Persistent Memory) wrote:
> 
> 
>> -----Original Message-----
>> From: Linux-nvdimm [mailto:linux-nvdimm-bounces@lists.01.org] On
>> Behalf Of Dave Jiang
>> Sent: Thursday, September 15, 2016 5:28 PM
>> To: vishal.l.verma@intel.com
>> Cc: linux-nvdimm@lists.01.org
>> Subject: [PATCH] ndctl: move test/dax-errors buffer to global to
>> avoid gcc optimization
>>
>> Some gcc toolchain are optimizing out the memcpy and this causes dax-
>> errors
>> to not trigger the SIG_BUS when doing memcpy on an mmap'd buffer. By
>> moving
>> the buffer to a global variable this bypasses the optimization and
>> allow
>> the test to work as intended.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>  test/dax-errors.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/test/dax-errors.c b/test/dax-errors.c
>> index 11d0031..9ea5c91 100644
>> --- a/test/dax-errors.c
>> +++ b/test/dax-errors.c
>> @@ -17,6 +17,8 @@
>>
>>  static sigjmp_buf sj_env;
>>  static int sig_count;
>> +/* buf is global in order to avoid gcc memcpy optimization */
>> +static void *buf;
>>
>>  static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
>>  {
>> @@ -27,7 +29,7 @@ static void sigbus_hdl(int sig, siginfo_t *siginfo,
>> void *ptr)
>>
>>  static int test_dax_read_err(int fd)
>>  {
>> -void *base, *buf;
>> +void *base;
>>  int rc = 0;
>>
>>  if (fd < 0) {
>>
> 
> I've run into that kind of problem before, and found that
> marking *buf as volatile (and leaving it inside the function)
> tends to be honored better by aggressive optimizing compilers
> and linkers.

Doesn't appear to work. The compiler discards the volatile.


  CC       dax-errors.o
dax-errors.c: In function ‘test_dax_read_err’:
dax-errors.c:66:9: warning: passing argument 1 of ‘memcpy’ discards
‘volatile’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  memcpy(buf, base, 4096);
         ^~~
In file included from dax-errors.c:8:0:
/usr/include/string.h:42:14: note: expected ‘void * restrict’ but
argument is of type ‘volatile void *’
 extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
              ^~~~~~
  CCLD     dax-errors

> 
> ---
> Robert Elliott, HPE Persistent Memory
> 
> 
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

RE: [PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Dave Jiang [mailto:dave.jiang@intel.com]
> Sent: Friday, September 16, 2016 12:24 PM
...
> Subject: Re: [PATCH] ndctl: move test/dax-errors buffer to global to
> avoid gcc optimization
> 
> On 09/15/2016 06:18 PM, Elliott, Robert (Persistent Memory) wrote:
> >
....
> >> Some gcc toolchain are optimizing out the memcpy and this causes
> >> dax- errors to not trigger the SIG_BUS when doing memcpy on an 
> >> mmap'd buffer.  By moving
> >> the buffer to a global variable this bypasses the optimization and
> >> allow the test to work as intended.
> >>
...
> >> diff --git a/test/dax-errors.c b/test/dax-errors.c
> >> index 11d0031..9ea5c91 100644
> >> --- a/test/dax-errors.c
> >> +++ b/test/dax-errors.c
> >> @@ -17,6 +17,8 @@
> >>
> >>  static sigjmp_buf sj_env;
> >>  static int sig_count;
> >> +/* buf is global in order to avoid gcc memcpy optimization */
> >> +static void *buf;
> >>
> >>  static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
> >>  {
> >> @@ -27,7 +29,7 @@ static void sigbus_hdl(int sig, siginfo_t *siginfo,
> >> void *ptr)
> >>
> >>  static int test_dax_read_err(int fd)
> >>  {
> >> -void *base, *buf;
> >> +void *base;
> >>  int rc = 0;
> >>
> >>  if (fd < 0) {
> >>
> >
> > I've run into that kind of problem before, and found that
> > marking *buf as volatile (and leaving it inside the function)
> > tends to be honored better by aggressive optimizing compilers
> > and linkers.
> 
> Doesn't appear to work. The compiler discards the volatile.
> 
> 
>   CC       dax-errors.o
> dax-errors.c: In function 'test_dax_read_err':
> dax-errors.c:66:9: warning: passing argument 1 of 'memcpy' discards
> 'volatile' qualifier from pointer target type [-Wdiscarded-
> qualifiers]
>   memcpy(buf, base, 4096);
>          ^~~
> In file included from dax-errors.c:8:0:
> /usr/include/string.h:42:14: note: expected 'void * restrict' but
> argument is of type 'volatile void *'
>  extern void *memcpy (void *__restrict __dest, const void *__restrict
> __src,
>               ^~~~~~
>   CCLD     dax-errors
> 

For this, put volatile to the right of the *:
	void * volatile buf;

That means buf (the pointer) is volatile, and the compiler is
not allowed to optimize away any accesses inside this function.
When calling another function, it just picks up the value at
that time; that other function doesn't need to continue treating
it as special.

On a little test program, gcc -O2 preserves the call, and -O3 
optimizes away the call unless it is marked volatile like that.

In comparison:
	volatile void *buf;
means whatever buf points to is volatile, and all functions to
which buf is passed must agree.  Accesses to buf itself could
be optimized away, but any dereferences using it that remain
must be freshly made.

memcpy does not promise to treat its buffers that way, so 
triggers compiler warnings or errors if src or dest are pointing
to volatile data.

In a test program, I made a mycpy() with volatile * buffer
arguments, and gcc -O3 does optimize away the call.

This combination has both properties:
	volatile void * volatile buf;


---
Robert Elliott, HPE Persistent Memory


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[Dave Jiang]

On 09/18/2016 09:42 AM, Elliott, Robert (Persistent Memory) wrote:
> 
> 
>> -----Original Message-----
>> From: Dave Jiang [mailto:dave.jiang@intel.com]
>> Sent: Friday, September 16, 2016 12:24 PM
> ...
>> Subject: Re: [PATCH] ndctl: move test/dax-errors buffer to global to
>> avoid gcc optimization
>>
>> On 09/15/2016 06:18 PM, Elliott, Robert (Persistent Memory) wrote:
>>>
> ....
>>>> Some gcc toolchain are optimizing out the memcpy and this causes
>>>> dax- errors to not trigger the SIG_BUS when doing memcpy on an
>>>> mmap'd buffer.  By moving
>>>> the buffer to a global variable this bypasses the optimization and
>>>> allow the test to work as intended.
>>>>
> ...
>>>> diff --git a/test/dax-errors.c b/test/dax-errors.c
>>>> index 11d0031..9ea5c91 100644
>>>> --- a/test/dax-errors.c
>>>> +++ b/test/dax-errors.c
>>>> @@ -17,6 +17,8 @@
>>>>
>>>>  static sigjmp_buf sj_env;
>>>>  static int sig_count;
>>>> +/* buf is global in order to avoid gcc memcpy optimization */
>>>> +static void *buf;
>>>>
>>>>  static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
>>>>  {
>>>> @@ -27,7 +29,7 @@ static void sigbus_hdl(int sig, siginfo_t *siginfo,
>>>> void *ptr)
>>>>
>>>>  static int test_dax_read_err(int fd)
>>>>  {
>>>> -void *base, *buf;
>>>> +void *base;
>>>>  int rc = 0;
>>>>
>>>>  if (fd < 0) {
>>>>
>>>
>>> I've run into that kind of problem before, and found that
>>> marking *buf as volatile (and leaving it inside the function)
>>> tends to be honored better by aggressive optimizing compilers
>>> and linkers.
>>
>> Doesn't appear to work. The compiler discards the volatile.
>>
>>
>>   CC       dax-errors.o
>> dax-errors.c: In function 'test_dax_read_err':
>> dax-errors.c:66:9: warning: passing argument 1 of 'memcpy' discards
>> 'volatile' qualifier from pointer target type [-Wdiscarded-
>> qualifiers]
>>   memcpy(buf, base, 4096);
>>          ^~~
>> In file included from dax-errors.c:8:0:
>> /usr/include/string.h:42:14: note: expected 'void * restrict' but
>> argument is of type 'volatile void *'
>>  extern void *memcpy (void *__restrict __dest, const void *__restrict
>> __src,
>>               ^~~~~~
>>   CCLD     dax-errors
>>
> 
> For this, put volatile to the right of the *:
> void * volatile buf;

It made the compile warning go away, but it did not prevent the memcpy
optimization. It still fails unlike when I move the ptr to global.

gcc version 6.1.1 20160621 (Red Hat 6.1.1-3) (GCC)

gcc -DHAVE_CONFIG_H -I. -I..  -include ../config.h
-DSYSCONFDIR=\""/etc"\" -DLIBEXECDIR=\""/usr/libexec"\"
-DPREFIX=\""/usr"\" -DNDCTL_MAN_PATH=\""/usr/share/man"\" -I../ndctl/lib
-I../ndctl -I../   -I/usr/include/uuid -I/usr/include/json-c  -Wall
-Wchar-subscripts -Wformat-security -Wmissing-declarations
-Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wshadow
-Wsign-compare -Wstrict-prototypes -Wtype-limits  -fvisibility=hidden
-ffunction-sections -fdata-sections -g -O2 -MT dax-errors.o -MD -MP -MF
$depbase.Tpo -c -o dax-errors.o dax-errors.c

> 
> That means buf (the pointer) is volatile, and the compiler is
> not allowed to optimize away any accesses inside this function.
> When calling another function, it just picks up the value at
> that time; that other function doesn't need to continue treating
> it as special.
> 
> On a little test program, gcc -O2 preserves the call, and -O3
> optimizes away the call unless it is marked volatile like that.
> 
> In comparison:
> volatile void *buf;
> means whatever buf points to is volatile, and all functions to
> which buf is passed must agree.  Accesses to buf itself could
> be optimized away, but any dereferences using it that remain
> must be freshly made.
> 
> memcpy does not promise to treat its buffers that way, so
> triggers compiler warnings or errors if src or dest are pointing
> to volatile data.
> 
> In a test program, I made a mycpy() with volatile * buffer
> arguments, and gcc -O3 does optimize away the call.
> 
> This combination has both properties:
> volatile void * volatile buf;
> 
> 
> ---
> Robert Elliott, HPE Persistent Memory
> 
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] ndctl: move test/dax-errors buffer to global to avoid gcc optimization

[Dan Williams]

On Mon, Sep 19, 2016 at 9:45 AM, Dave Jiang <dave.jiang@intel.com> wrote:
> On 09/18/2016 09:42 AM, Elliott, Robert (Persistent Memory) wrote:
>>
>>
>>> -----Original Message-----
>>> From: Dave Jiang [mailto:dave.jiang@intel.com]
>>> Sent: Friday, September 16, 2016 12:24 PM
>> ...
>>> Subject: Re: [PATCH] ndctl: move test/dax-errors buffer to global to
>>> avoid gcc optimization
>>>
>>> On 09/15/2016 06:18 PM, Elliott, Robert (Persistent Memory) wrote:
>>>>
>> ....
>>>>> Some gcc toolchain are optimizing out the memcpy and this causes
>>>>> dax- errors to not trigger the SIG_BUS when doing memcpy on an
>>>>> mmap'd buffer.  By moving
>>>>> the buffer to a global variable this bypasses the optimization and
>>>>> allow the test to work as intended.
>>>>>
>> ...
>>>>> diff --git a/test/dax-errors.c b/test/dax-errors.c
>>>>> index 11d0031..9ea5c91 100644
>>>>> --- a/test/dax-errors.c
>>>>> +++ b/test/dax-errors.c
>>>>> @@ -17,6 +17,8 @@
>>>>>
>>>>>  static sigjmp_buf sj_env;
>>>>>  static int sig_count;
>>>>> +/* buf is global in order to avoid gcc memcpy optimization */
>>>>> +static void *buf;
>>>>>
>>>>>  static void sigbus_hdl(int sig, siginfo_t *siginfo, void *ptr)
>>>>>  {
>>>>> @@ -27,7 +29,7 @@ static void sigbus_hdl(int sig, siginfo_t *siginfo,
>>>>> void *ptr)
>>>>>
>>>>>  static int test_dax_read_err(int fd)
>>>>>  {
>>>>> -void *base, *buf;
>>>>> +void *base;
>>>>>  int rc = 0;
>>>>>
>>>>>  if (fd < 0) {
>>>>>
>>>>
>>>> I've run into that kind of problem before, and found that
>>>> marking *buf as volatile (and leaving it inside the function)
>>>> tends to be honored better by aggressive optimizing compilers
>>>> and linkers.
>>>
>>> Doesn't appear to work. The compiler discards the volatile.
>>>
>>>
>>>   CC       dax-errors.o
>>> dax-errors.c: In function 'test_dax_read_err':
>>> dax-errors.c:66:9: warning: passing argument 1 of 'memcpy' discards
>>> 'volatile' qualifier from pointer target type [-Wdiscarded-
>>> qualifiers]
>>>   memcpy(buf, base, 4096);
>>>          ^~~
>>> In file included from dax-errors.c:8:0:
>>> /usr/include/string.h:42:14: note: expected 'void * restrict' but
>>> argument is of type 'volatile void *'
>>>  extern void *memcpy (void *__restrict __dest, const void *__restrict
>>> __src,
>>>               ^~~~~~
>>>   CCLD     dax-errors
>>>
>>
>> For this, put volatile to the right of the *:
>> void * volatile buf;
>
> It made the compile warning go away, but it did not prevent the memcpy
> optimization. It still fails unlike when I move the ptr to global.
>
> gcc version 6.1.1 20160621 (Red Hat 6.1.1-3) (GCC)
>
> gcc -DHAVE_CONFIG_H -I. -I..  -include ../config.h
> -DSYSCONFDIR=\""/etc"\" -DLIBEXECDIR=\""/usr/libexec"\"
> -DPREFIX=\""/usr"\" -DNDCTL_MAN_PATH=\""/usr/share/man"\" -I../ndctl/lib
> -I../ndctl -I../   -I/usr/include/uuid -I/usr/include/json-c  -Wall
> -Wchar-subscripts -Wformat-security -Wmissing-declarations
> -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wshadow
> -Wsign-compare -Wstrict-prototypes -Wtype-limits  -fvisibility=hidden
> -ffunction-sections -fdata-sections -g -O2 -MT dax-errors.o -MD -MP -MF
> $depbase.Tpo -c -o dax-errors.o dax-errors.c

I've already applied the "make it global" version.  So we can defer
more "volatile" keyword experiments.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm