Skip to contentSkip to conversation

s390: include: timex: Use macro CLOCK_STORE_SIZE instead of hard code number

PATCH In review Chen Gang 3 participants 7 messages Jan 1, 2015 – Jan 9, 2015 s390

lore
Share & tools
lore

Chen Gang

author
#0 lore raw

PATCH 1/1 s390: include: timex: Use macro CLOCK_STORE_SIZE instead of hard code number

For C language, it treats array parameter as a pointer, so sizeof for an array parameter is equal to sizeof for a pointer, which causes compiler warning (with allmodconfig by gcc 5):

CC arch/s390/kernel/asm-offsets.s
In file included from include/linux/timex.h:65:0,

                   from include/linux/sched.h:19,
                   from include/linux/kvm_host.h:15,
                   from arch/s390/kernel/asm-offsets.c:10:
  ./arch/s390/include/asm/timex.h: In function 'get_tod_clock_ext':
  ./arch/s390/include/asm/timex.h:76:32: warning: 'sizeof' on array function parameter 'clk' will return size of 'char *' [-Wsizeof-array-argument]
    typedef struct { char _[sizeof(clk)]; } addrtype;
                                  ^

Can use macro CLOCK_STORE_SIZE instead of all related hard code numbers, which also can avoid this warning. And also add a tab to CLOCK_TICK_RATE definition to match coding styles.

Signed-off-by Chen Gang <[email protected]>
---
 arch/s390/hypfs/hypfs_vm.c    |    2 +-
 arch/s390/include/asm/timex.h |    9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)
arch/s390/hypfs/hypfs_vm.c +1 -1
index 32040ac..344b622 100644
@@ -231,7 +231,7 @@ failed:
 struct dbfs_d2fc_hdr {
 	u64	len;		/* Length of d2fc buffer without header */
 	u16	version;	/* Version of header */
-	char	tod_ext[16];	/* TOD clock for d2fc */
+	char	tod_ext[CLOCK_STORE_SIZE]; /* TOD clock for d2fc */
 	u64	count;		/* Number of VM guests in d2fc buffer */
 	char	reserved[30];
 } __attribute__ ((packed));
arch/s390/include/asm/timex.h +5 -4
index 8beee1c..2d2fab6 100644
@@ -67,20 +67,21 @@ static inline void local_tick_enable(unsigned long long comp)
 	set_clock_comparator(S390_lowcore.clock_comparator);
 }
 
-#define CLOCK_TICK_RATE	1193180 /* Underlying HZ */
+#define CLOCK_TICK_RATE		1193180 /* Underlying HZ */
+#define CLOCK_STORE_SIZE	16 /* store size for clock */
 
 typedef unsigned long long cycles_t;
 
-static inline void get_tod_clock_ext(char clk[16])
+static inline void get_tod_clock_ext(char clk[CLOCK_STORE_SIZE])
 {
-	typedef struct { char _[sizeof(clk)]; } addrtype;
+	typedef struct { char _[CLOCK_STORE_SIZE]; } addrtype;
 
 	asm volatile("stcke %0" : "=Q" (*(addrtype *) clk) : : "cc");
 }
 
 static inline unsigned long long get_tod_clock(void)
 {
-	unsigned char clk[16];
+	unsigned char clk[CLOCK_STORE_SIZE];
 	get_tod_clock_ext(clk);
 	return *((unsigned long long *)&clk[1]);
 }
Signature · 1 line
1.7.9.5
#1 lore raw
Quoted 14 lines · Chen Gang

For C language, it treats array parameter as a pointer, so sizeof for an array parameter is equal to sizeof for a pointer, which causes compiler warning (with allmodconfig by gcc 5):

CC arch/s390/kernel/asm-offsets.s
In file included from include/linux/timex.h:65:0,

                   from include/linux/sched.h:19,
                   from include/linux/kvm_host.h:15,
                   from arch/s390/kernel/asm-offsets.c:10:
  ./arch/s390/include/asm/timex.h: In function 'get_tod_clock_ext':
  ./arch/s390/include/asm/timex.h:76:32: warning: 'sizeof' on array function parameter 'clk' will return size of 'char *' [-Wsizeof-array-argument]
    typedef struct { char _[sizeof(clk)]; } addrtype;
                                  ^
Quoted 5 lines

Can use macro CLOCK_STORE_SIZE instead of all related hard code numbers, which also can avoid this warning. And also add a tab to CLOCK_TICK_RATE definition to match coding styles.

Signed-off-by Chen Gang <[email protected]>

Thanks. I applied the slightly changed version below.

From 77e1e6fd8f5ce0d96c035056f9e963ca7d9198b2 Mon Sep 17 00:00:00 2001
From: Chen Gang <[email protected]>
Date: Thu, 1 Jan 2015 22:27:32 +0800
Subject: [PATCH] s390/timex: fix get_tod_clock_ext() inline assembly

For C language, it treats array parameter as a pointer, so sizeof for an array parameter is equal to sizeof for a pointer, which causes compiler warning (with allmodconfig by gcc 5):

./arch/s390/include/asm/timex.h: In function 'get_tod_clock_ext':
./arch/s390/include/asm/timex.h:76:32: warning: 'sizeof' on array function parameter 'clk' will return size of 'char *' [-Wsizeof-array-argument]

    typedef struct { char _[sizeof(clk)]; } addrtype;
                                  ^

Can use macro CLOCK_STORE_SIZE instead of all related hard code numbers, which also can avoid this warning. And also add a tab to CLOCK_TICK_RATE definition to match coding styles.

[[email protected]]:
Chen's patch actually fixes a bug within the get_tod_clock_ext() inline assembly where we incorrectly tell the compiler that only 8 bytes of memory get changed instead of 16 bytes.
This would allow gcc to generate incorrect code. Right now this doesn't seem to be the case.
Also slightly changed the patch a bit.
- renamed CLOCK_STORE_SIZE to STORE_CLOCK_SIZE
- changed get_tod_clock_ext() to receive a char pointer parameter

Signed-off-by Chen Gang <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>
---
 arch/s390/hypfs/hypfs_vm.c    |  2 +-
 arch/s390/include/asm/timex.h | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)
arch/s390/hypfs/hypfs_vm.c +1 -1
index 32040ace00ea..99a3e6b395ab 100644
@@ -231,7 +231,7 @@ failed:
 struct dbfs_d2fc_hdr {
 	u64	len;		/* Length of d2fc buffer without header */
 	u16	version;	/* Version of header */
-	char	tod_ext[16];	/* TOD clock for d2fc */
+	char	tod_ext[STORE_CLOCK_SIZE]; /* TOD clock for d2fc */
 	u64	count;		/* Number of VM guests in d2fc buffer */
 	char	reserved[30];
 } __attribute__ ((packed));
arch/s390/include/asm/timex.h +6 -4
index 8beee1cceba4..582be106ab4a 100644
@@ -67,20 +67,22 @@ static inline void local_tick_enable(unsigned long long comp)
 	set_clock_comparator(S390_lowcore.clock_comparator);
 }
 
-#define CLOCK_TICK_RATE	1193180 /* Underlying HZ */
+#define CLOCK_TICK_RATE		1193180 /* Underlying HZ */
+#define STORE_CLOCK_SIZE	16	/* number of bytes store clock writes */
 
 typedef unsigned long long cycles_t;
 
-static inline void get_tod_clock_ext(char clk[16])
+static inline void get_tod_clock_ext(char *clk)
 {
-	typedef struct { char _[sizeof(clk)]; } addrtype;
+	typedef struct { char _[STORE_CLOCK_SIZE]; } addrtype;
 
 	asm volatile("stcke %0" : "=Q" (*(addrtype *) clk) : : "cc");
 }
 
 static inline unsigned long long get_tod_clock(void)
 {
-	unsigned char clk[16];
+	unsigned char clk[STORE_CLOCK_SIZE];
+
 	get_tod_clock_ext(clk);
 	return *((unsigned long long *)&clk[1]);
 }
Signature · 1 line
1.8.5.6
author
#2 lore raw

Thank you for your work.

In honest, originally, I was not sure whether it would cause bug (do not know gcc would generic incorrect code for it). :-)

Thanks.

Quoted 102 lines · Heiko Carstens
Quoted 14 lines · Chen Gang

For C language, it treats array parameter as a pointer, so sizeof for an array parameter is equal to sizeof for a pointer, which causes compiler warning (with allmodconfig by gcc 5):

CC arch/s390/kernel/asm-offsets.s
In file included from include/linux/timex.h:65:0,

                   from include/linux/sched.h:19,
                   from include/linux/kvm_host.h:15,
                   from arch/s390/kernel/asm-offsets.c:10:
  ./arch/s390/include/asm/timex.h: In function 'get_tod_clock_ext':
  ./arch/s390/include/asm/timex.h:76:32: warning: 'sizeof' on array function parameter 'clk' will return size of 'char *' [-Wsizeof-array-argument]
    typedef struct { char _[sizeof(clk)]; } addrtype;
                                  ^
Quoted 5 lines

Can use macro CLOCK_STORE_SIZE instead of all related hard code numbers, which also can avoid this warning. And also add a tab to CLOCK_TICK_RATE definition to match coding styles.

Signed-off-by Chen Gang <[email protected]>

Thanks. I applied the slightly changed version below.

Quoted 1 line
Quoted 1 line

From 77e1e6fd8f5ce0d96c035056f9e963ca7d9198b2 Mon Sep 17 00:00:00 2001

From: Chen Gang <[email protected]>
Date: Thu, 1 Jan 2015 22:27:32 +0800
Subject: [PATCH] s390/timex: fix get_tod_clock_ext() inline assembly

For C language, it treats array parameter as a pointer, so sizeof for an array parameter is equal to sizeof for a pointer, which causes compiler warning (with allmodconfig by gcc 5):

./arch/s390/include/asm/timex.h: In function 'get_tod_clock_ext':
./arch/s390/include/asm/timex.h:76:32: warning: 'sizeof' on array function parameter 'clk' will return size of 'char *' [-Wsizeof-array-argument]

    typedef struct { char _[sizeof(clk)]; } addrtype;
                                  ^

Can use macro CLOCK_STORE_SIZE instead of all related hard code numbers, which also can avoid this warning. And also add a tab to CLOCK_TICK_RATE definition to match coding styles.

[[email protected]]:
Chen's patch actually fixes a bug within the get_tod_clock_ext() inline assembly where we incorrectly tell the compiler that only 8 bytes of memory get changed instead of 16 bytes.
This would allow gcc to generate incorrect code. Right now this doesn't seem to be the case.
Also slightly changed the patch a bit.
- renamed CLOCK_STORE_SIZE to STORE_CLOCK_SIZE
- changed get_tod_clock_ext() to receive a char pointer parameter

Signed-off-by Chen Gang <[email protected]>
Signed-off-by Heiko Carstens <[email protected]>
---
 arch/s390/hypfs/hypfs_vm.c    |  2 +-
 arch/s390/include/asm/timex.h | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)
arch/s390/hypfs/hypfs_vm.c +1 -1
index 32040ace00ea..99a3e6b395ab 100644
@@ -231,7 +231,7 @@ failed:
 struct dbfs_d2fc_hdr {
 	u64	len;		/* Length of d2fc buffer without header */
 	u16	version;	/* Version of header */
-	char	tod_ext[16];	/* TOD clock for d2fc */
+	char	tod_ext[STORE_CLOCK_SIZE]; /* TOD clock for d2fc */
 	u64	count;		/* Number of VM guests in d2fc buffer */
 	char	reserved[30];
 } __attribute__ ((packed));
arch/s390/include/asm/timex.h +6 -4
index 8beee1cceba4..582be106ab4a 100644
@@ -67,20 +67,22 @@ static inline void local_tick_enable(unsigned long long comp)
 	set_clock_comparator(S390_lowcore.clock_comparator);
 }
 
-#define CLOCK_TICK_RATE	1193180 /* Underlying HZ */
+#define CLOCK_TICK_RATE		1193180 /* Underlying HZ */
+#define STORE_CLOCK_SIZE	16	/* number of bytes store clock writes */
 
 typedef unsigned long long cycles_t;
 
-static inline void get_tod_clock_ext(char clk[16])
+static inline void get_tod_clock_ext(char *clk)
 {
-	typedef struct { char _[sizeof(clk)]; } addrtype;
+	typedef struct { char _[STORE_CLOCK_SIZE]; } addrtype;
 
 	asm volatile("stcke %0" : "=Q" (*(addrtype *) clk) : : "cc");
 }
 
 static inline unsigned long long get_tod_clock(void)
 {
-	unsigned char clk[16];
+	unsigned char clk[STORE_CLOCK_SIZE];
+
 	get_tod_clock_ext(clk);
 	return *((unsigned long long *)&clk[1]);
 }
Signature · 3 lines
Chen Gang

Open, share, and attitude like air, water, and life which God blessed
#3 lore raw
Quoted 5 lines · On Sat, 03 Jan 2015 11:44:04 +0800 Chen Gang <[email protected]> wrote:

Thank you for your work.

In honest, originally, I was not sure whether it would cause bug (do not know gcc would generic incorrect code for it). :-)

Even if the code happened to be correct it does not matter. The intention of the sizeof() has been to get to the correct 16, not 8. The fix is fine as it is.

Signature · 4 lines
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.
author
#4 lore raw
Quoted 4 lines · Martin Schwidefsky
Quoted 1 line · On Sat, 03 Jan 2015 11:44:04 +0800 Chen Gang <[email protected]> wrote:
Quoted 9 lines
Quoted 4 lines

Thank you for your work.

In honest, originally, I was not sure whether it would cause bug (do not know gcc would generic incorrect code for it). :-)

Even if the code happened to be correct it does not matter. The intention of the sizeof() has been to get to the correct 16, not 8. The fix is fine as it is.

Excuse me, my English is not quite well, I am not quite sure about what you said (might misunderstand what you said), so I provide the related information below for confirmation, please check, thanks.

sizeof(clk) is for a pointer, not for an array (for C language, it treats array parameter as a pointer), the related demo is below:

  [root@localhost test]# cat ./test.c
  #include <stdio.h>
  
  static inline void get_tod_clock_ext(char clk[16])
  {
  	typedef struct { char _[sizeof(clk)]; } addrtype;
  
  	printf("\nsize: %ld\n", sizeof(addrtype));
  }
  
  int main()
  {
  	char clk[16];
  
  	get_tod_clock_ext(clk);
  	return 0;
  }
  [root@localhost test]# cc -Wall -O2 -o test test.c
  [root@localhost test]# ./test

size: 8
[root@localhost test]# cc -v
Using built-in specs.
COLLECT_GCC=cc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.8.3/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin --enable-initfini-array --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-isl=/builddir/build/BUILD/gcc-4.8.3-20140624/obj-x86_64-redhat-linux/isl-install --with-cloog=/builddir/build/BUILD/gcc-4.8.3-20140624/obj-x86_64-redhat-linux/cloog-install --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux Thread model: posix
gcc version 4.8.3 20140624 (Red Hat 4.8.3-1) (GCC)
[root@localhost test]#

Thanks.

Signature · 1 line
Open, share, and attitude like air, water, and life which God blessed.
#5 lore raw
Quoted 15 lines · On Wed, 07 Jan 2015 22:45:11 +0800 Chen Gang S <[email protected]> wrote:
Quoted 4 lines · Martin Schwidefsky
Quoted 1 line · On Sat, 03 Jan 2015 11:44:04 +0800 Chen Gang <[email protected]> wrote:
Quoted 9 lines
Quoted 4 lines

Thank you for your work.

In honest, originally, I was not sure whether it would cause bug (do not know gcc would generic incorrect code for it). :-)

Even if the code happened to be correct it does not matter. The intention of the sizeof() has been to get to the correct 16, not 8. The fix is fine as it is.

Quoted 6 lines

Excuse me, my English is not quite well, I am not quite sure about what you said (might misunderstand what you said), so I provide the related information below for confirmation, please check, thanks.

sizeof(clk) is for a pointer, not for an array (for C language, it treats array parameter as a pointer), the related demo is below:

And your patch fixes this problem. My comment was in regard to the impact of the original bug. As the typeof construct is used to prevent the compiler from over-optimizing, the code can come out correct even if the bug is present.

Signature · 4 lines
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.
author
#6 lore raw
Quoted 22 lines · Martin Schwidefsky
Quoted 19 lines · On Wed, 07 Jan 2015 22:45:11 +0800 Chen Gang S <[email protected]> wrote:
Quoted 4 lines · Martin Schwidefsky
Quoted 1 line · On Sat, 03 Jan 2015 11:44:04 +0800 Chen Gang <[email protected]> wrote:
Quoted 9 lines
Quoted 4 lines

Thank you for your work.

In honest, originally, I was not sure whether it would cause bug (do not know gcc would generic incorrect code for it). :-)

Even if the code happened to be correct it does not matter. The intention of the sizeof() has been to get to the correct 16, not 8. The fix is fine as it is.

Excuse me, my English is not quite well, I am not quite sure about what you said (might misunderstand what you said), so I provide the related information below for confirmation, please check, thanks.

Quoted 8 lines
Quoted 2 lines

sizeof(clk) is for a pointer, not for an array (for C language, it treats array parameter as a pointer), the related demo is below:

And your patch fixes this problem. My comment was in regard to the impact of the original bug. As the typeof construct is used to prevent the compiler from over-optimizing, the code can come out correct even if the bug is present.

OK, thank you for your reply.

In honest, I still not quite understand your meaning, I guess it is only because of my poor English, and it doesn't matter for others members, so not need additional reply for it (but welcome reply).

For details (please check, if still interest):

Quoted 2 lines

And your patch fixes this problem. My comment was in regard to the impact of the original bug.

I can understand the 2 contents above.

Quoted 2 lines

As the typeof construct is used to
prevent the compiler from over-optimizing,

I can understand, yeah, typeof() is useful for declaring an array.

Quoted 3 lines

the code can come out
correct even if the bug is present.

Sorry, I can not understand: I know every words, and it seems I can understand the whole sentence, but for me, it seems have a conflict meaning with the original sentences.

Thanks.

Signature · 3 lines
Chen Gang

Open, share, and attitude like air, water, and life which God blessed