eal/windows: resolve conversion and truncation warnings

PATCH Superseded Tyler Retzlaff 3 participants 8 messages Aug 2, 2023 – Mar 7, 2024

Share & tools

Tyler Retzlaff

author

2023-08-02 20:48 UTC #0 lore raw

PATCH 1/1 eal/windows: resolve conversion and truncation warnings

* Initialize const int NS_PER_SEC with an integer literal instead of double thereby avoiding implicit conversion from double to int.

* Cast the result of the expression assigned to timspec.tv_nsec to long. Windows builds generate integer truncation warning for this assignment since the result of the expression was 8 bytes (LONGLONG) but on Windows targets is 4 bytes. The value produced for the expression should safely fit in the long.

Signed-off-by Tyler Retzlaff <[email protected]>

---
 lib/eal/windows/include/rte_os_shim.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

lib/eal/windows/include/rte_os_shim.h +2 -2

index eda8113..19b12e9 100644
@@ -87,7 +87,7 @@
 static inline int
 rte_clock_gettime(clockid_t clock_id, struct timespec *tp)
 {
-	const int NS_PER_SEC = 1E9;
+	const int NS_PER_SEC = 1000000000;
 	LARGE_INTEGER pf, pc;
 	LONGLONG nsec;

@@ -102,7 +102,7 @@
 
 		nsec = pc.QuadPart * NS_PER_SEC / pf.QuadPart;
 		tp->tv_sec = nsec / NS_PER_SEC;
-		tp->tv_nsec = nsec - tp->tv_sec * NS_PER_SEC;
+		tp->tv_nsec = (long)(nsec - tp->tv_sec * NS_PER_SEC);
 		return 0;
 	default:
 		return -1;

Signature · 1 line

1.8.3.1

Dmitry Kozlyuk

Acked-by on v1 1/1

2023-08-02 22:29 UTC #1 lore raw

2023-08-02 13:48 (UTC-0700), Tyler Retzlaff:

Quoted 4 lines

* Initialize const int NS_PER_SEC with an integer literal instead of double thereby avoiding implicit conversion from double to int.
* Cast the result of the expression assigned to timspec.tv_nsec to long.

Typo: "timespec".

Quoted 3 lines

Windows builds generate integer truncation warning for this assignment since the result of the expression was 8 bytes (LONGLONG) but on Windows targets is 4 bytes.

Probably "but **tv_nsec** on Windows targets is 4 bytes".

Quoted 6 lines

The value produced for the expression should safely fit in the long.
Signed-off-by Tyler Retzlaff <[email protected]>
---
 lib/eal/windows/include/rte_os_shim.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by Dmitry Kozlyuk <[email protected]>

Tyler Retzlaff

author

2023-08-02 22:41 UTC #2 lore raw

Quoted 7 lines · Dmitry Kozlyuk

2023-08-02 13:48 (UTC-0700), Tyler Retzlaff:
Quoted 4 lines
* Initialize const int NS_PER_SEC with an integer literal instead of double thereby avoiding implicit conversion from double to int.
* Cast the result of the expression assigned to timspec.tv_nsec to long.
Typo: "timespec".

oops

Quoted 6 lines

Quoted 4 lines
Windows builds generate integer truncation warning for this assignment since the result of the expression was 8 bytes (LONGLONG) but on Windows targets is 4 bytes.
Probably "but **tv_nsec** on Windows targets is 4 bytes".

thanks i'll update the wording.

one thing that confuses me a little and this change won't break how the code already works (just makes the cast redundant) is that for mingw sizeof(long) is being reported as 8 bytes.

this is in spec relative to the C standard but it does leave me somewhat concerned if struct timespec as defined in the windows headers crosses an abi boundary.

have you ever noticed this? any thoughts on it?

Quoted 9 lines

Quoted 7 lines
The value produced for the expression should safely fit in the long.
Signed-off-by Tyler Retzlaff <[email protected]>
---
 lib/eal/windows/include/rte_os_shim.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Acked-by Dmitry Kozlyuk <[email protected]>

thanks!

Dmitry Kozlyuk

2023-08-02 23:44 UTC #3 lore raw

2023-08-02 15:41 (UTC-0700), Tyler Retzlaff:

Quoted 7 lines

one thing that confuses me a little and this change won't break how the code already works (just makes the cast redundant) is that for mingw sizeof(long) is being reported as 8 bytes.
this is in spec relative to the C standard but it does leave me somewhat concerned if struct timespec as defined in the windows headers crosses an abi boundary.

MinGW-w64 shows sizeof(long) == 4 in my tests, both native and cross build. Which MinGW setup reports sizeof(long) == 8 on Windows target?

Tyler Retzlaff

author

2023-08-03 00:30 UTC #4 lore raw

Quoted 11 lines · Dmitry Kozlyuk

2023-08-02 15:41 (UTC-0700), Tyler Retzlaff:
Quoted 7 lines
one thing that confuses me a little and this change won't break how the code already works (just makes the cast redundant) is that for mingw sizeof(long) is being reported as 8 bytes.
this is in spec relative to the C standard but it does leave me somewhat concerned if struct timespec as defined in the windows headers crosses an abi boundary.
MinGW-w64 shows sizeof(long) == 4 in my tests, both native and cross build. Which MinGW setup reports sizeof(long) == 8 on Windows target?

it must have been a dream, i just checked and i get the results you do.

ignore me i'm tired, thanks for checking though.

Tyler Retzlaff

author

2024-03-07 18:34 UTC #5 lore raw

PATCH v2 1/1 eal/windows: resolve conversion and truncation warnings

In reply to Tyler Retzlaff

* Initialize const int NS_PER_SEC with an integer literal instead of double thereby avoiding implicit conversion from double to int.

* Cast the result of the expression assigned to timespec.tv_nsec to long.

Signed-off-by Tyler Retzlaff <[email protected]>

Acked-by Dmitry Kozlyuk <[email protected]>

---

v2:
* update commit message to correct misspelled timspec -> timespec, remove remarks about casting to long they were unnecessary.

 lib/eal/windows/include/rte_os_shim.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

lib/eal/windows/include/rte_os_shim.h +2 -2

index eda8113..19b12e9 100644
@@ -87,7 +87,7 @@
 static inline int
 rte_clock_gettime(clockid_t clock_id, struct timespec *tp)
 {
-	const int NS_PER_SEC = 1E9;
+	const int NS_PER_SEC = 1000000000;
 	LARGE_INTEGER pf, pc;
 	LONGLONG nsec;

@@ -102,7 +102,7 @@
 
 		nsec = pc.QuadPart * NS_PER_SEC / pf.QuadPart;
 		tp->tv_sec = nsec / NS_PER_SEC;
-		tp->tv_nsec = nsec - tp->tv_sec * NS_PER_SEC;
+		tp->tv_nsec = (long)(nsec - tp->tv_sec * NS_PER_SEC);
 		return 0;
 	default:
 		return -1;

Signature · 1 line

1.8.3.1

Bruce Richardson

2024-03-07 20:54 UTC #6 lore raw

Quoted 21 lines · Tyler Retzlaff

* Initialize const int NS_PER_SEC with an integer literal instead of double thereby avoiding implicit conversion from double to int.
* Cast the result of the expression assigned to timespec.tv_nsec to long.
Signed-off-by Tyler Retzlaff <[email protected]>
Acked-by Dmitry Kozlyuk <[email protected]>
---
v2:
* update commit message to correct misspelled timspec -> timespec, remove remarks about casting to long they were unnecessary.
 lib/eal/windows/include/rte_os_shim.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
lib/eal/windows/include/rte_os_shim.h +0 -0
index eda8113..19b12e9 100644

lib/eal/windows/include/rte_os_shim.h line 90

Quoted 5 lines

lib/eal/windows/include/rte_os_shim.h +1 -1

@@ -87,7 +87,7 @@
 static inline int
 rte_clock_gettime(clockid_t clock_id, struct timespec *tp)
 {
-	const int NS_PER_SEC = 1E9;
+	const int NS_PER_SEC = 1000000000;

Just for readability, and the immediate visibility of errors, could this be rewritten as (1000 * 1000 * 1000). That avoids us having to count the zeros to know that the number is correct.

BTW: is "int" still the best type to use for this value? Would it be better as a #define?

/Bruce

Quoted 15 lines

lib/eal/windows/include/rte_os_shim.h +1 -1

 	LARGE_INTEGER pf, pc;
 	LONGLONG nsec;

@@ -102,7 +102,7 @@
 
 		nsec = pc.QuadPart * NS_PER_SEC / pf.QuadPart;
 		tp->tv_sec = nsec / NS_PER_SEC;
-		tp->tv_nsec = nsec - tp->tv_sec * NS_PER_SEC;
+		tp->tv_nsec = (long)(nsec - tp->tv_sec * NS_PER_SEC);
 		return 0;
 	default:
 		return -1;

Signature · 1 line

1.8.3.1

Tyler Retzlaff

author

2024-03-07 20:57 UTC #7 lore raw

Quoted 32 lines · Bruce Richardson

Quoted 21 lines · Tyler Retzlaff
* Initialize const int NS_PER_SEC with an integer literal instead of double thereby avoiding implicit conversion from double to int.
* Cast the result of the expression assigned to timespec.tv_nsec to long.
Signed-off-by Tyler Retzlaff <[email protected]>
Acked-by Dmitry Kozlyuk <[email protected]>
---
v2:
* update commit message to correct misspelled timspec -> timespec, remove remarks about casting to long they were unnecessary.
 lib/eal/windows/include/rte_os_shim.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
lib/eal/windows/include/rte_os_shim.h +0 -0
index eda8113..19b12e9 100644
lib/eal/windows/include/rte_os_shim.h line 90
Quoted 5 lines
lib/eal/windows/include/rte_os_shim.h +1 -1
@@ -87,7 +87,7 @@
 static inline int
 rte_clock_gettime(clockid_t clock_id, struct timespec *tp)
 {
-	const int NS_PER_SEC = 1E9;
+	const int NS_PER_SEC = 1000000000;
Just for readability, and the immediate visibility of errors, could this be rewritten as (1000 * 1000 * 1000). That avoids us having to count the zeros to know that the number is correct.

Quoted 2 lines

BTW: is "int" still the best type to use for this value? Would it be better as a #define?

i think to save spot fixing i'm going to withdraw the series for now. i need to come back later and deal with warnings from MSVC more comprehensively anyway.

thanks folks!

Quoted 18 lines

/Bruce

Quoted 15 lines

lib/eal/windows/include/rte_os_shim.h +1 -1

 	LARGE_INTEGER pf, pc;
 	LONGLONG nsec;

@@ -102,7 +102,7 @@
 
 		nsec = pc.QuadPart * NS_PER_SEC / pf.QuadPart;
 		tp->tv_sec = nsec / NS_PER_SEC;
-		tp->tv_nsec = nsec - tp->tv_sec * NS_PER_SEC;
+		tp->tv_nsec = (long)(nsec - tp->tv_sec * NS_PER_SEC);
 		return 0;
 	default:
 		return -1;

Signature · 1 line

1.8.3.1