1256473 - netwerk/base/nsSocketTransport2.cpp(3029): warning C4838: conversion from 'int' to 'ULONG' requires a narrowing conversion

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Gregory Szorc [:gps]]

This warning is turning into an error in automation when building with VS2015u1.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?honza

As part of unblocking building with VS2015u1 in automation, I'm mass
disabling compiler warnings that are turned into errors. This is not
the preferred mechanism to fix compilation warnings. So hopefully
this patch never lands because someone insists of fixing the underlying
problem instead. But if it does land, hopefully the workaround is
only temporary.

Review commit: https://reviewboard.mozilla.org/r/39855/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39855/

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1256473 - Disable C4456 and C4458 to unblock compilation on VS2015; r?dkeeler

As part of unblocking building with VS2015u1 in automation, I'm mass
disabling compiler warnings that are turned into errors. This is not
the preferred mechanism to fix compilation warnings. So hopefully
someone fixes the underlying problem someday. However, there are tons
of ignored warnings in security/certverifier, so I guess the workaround
in this patch is par for the course.

Review commit: https://reviewboard.mozilla.org/r/39865/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39865/

Comment 3

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?mayhemer

As part of unblocking building with VS2015u1 in automation, I'm mass
disabling compiler warnings that are turned into errors. This is not
the preferred mechanism to fix compilation warnings. So hopefully
this patch never lands because someone insists of fixing the underlying
problem instead. But if it does land, hopefully the workaround is
only temporary.

Review commit: https://reviewboard.mozilla.org/r/39867/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39867/

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8730441 [details]
MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?mayhemer

You want to ask Jan Bambas (:mayhemer) for review I guess.
Honza

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8730441 [details]
MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?mayhemer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39867/diff/1-2/

Comment 6

[Honza Bambas (:mayhemer)]

Can you give me an example(s) in the code that is causing this warning?  Just to asses how big a problem we are hiding (if any).

Comment 7

[Gregory Szorc [:gps]]

The code in the bug summary is exhibiting the problem. Although the line number may have shifted.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27083abb8d05 should report the warning in <1hr.

Comment 8

[Gregory Szorc [:gps]]

The Try push reproduced the warning.

Comment 9

[Doug Turner (:dougt)]

can you help move this forward mcmanus?

Comment 10

[Honza Bambas (:mayhemer)]

OK, seems like VS2015 uncovers a lot of problems we have.  The number of warnings you have to disable is, I could say, massive, when looking at bug 1124033 and its dependencies.

Tho, I don't much follow why conversion from int to ULONG should be explicitly narrowing on a 32 bit build.

Anyway, if this is the plan to support VS2015, then, however I'm nor super happy about it, no need to block here.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8730441 [details]
MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?mayhemer

https://reviewboard.mozilla.org/r/39867/#review37557

Comment 12

[Patrick McManus [:mcmanus]]

I just see the one warning in that log..

:gps, do you mind seeing if this patch clears it up without the compiler directive? If not, I'm fine with adding the compiler switch

diff --git a/netwerk/base/nsSocketTransport2.cpp b/netwerk/base/nsSocketTransport2.cpp
index 34165ec..fcdf005 100644
--- a/netwerk/base/nsSocketTransport2.cpp
+++ b/netwerk/base/nsSocketTransport2.cpp
@@ -3017,20 +3017,20 @@ nsSocketTransport::PRFileDescAutoLock::SetKeepaliveVals(bool aEnabled,
                      mSocketTransport);
         return ErrorAccordingToNSPR(PR_GetError());
     }
 #endif
 
 #if defined(XP_WIN)
     // Windows allows idle time and retry interval to be set; NOT ping count.
     struct tcp_keepalive keepalive_vals = {
-        (int)aEnabled,
+        (u_long)aEnabled,
         // Windows uses msec.
-        aIdleTime * 1000,
-        aRetryInterval * 1000
+        (u_long)(aIdleTime * 1000),
+        (u_long)(aRetryInterval * 1000)
     };
     DWORD bytes_returned;
     int err = WSAIoctl(sock, SIO_KEEPALIVE_VALS, &keepalive_vals,
                        sizeof(keepalive_vals), NULL, 0, &bytes_returned, NULL,
                        NULL);
     if (NS_WARN_IF(err)) {
         LogOSError("nsSocketTransport WSAIoctl failed", mSocketTransport);
         return NS_ERROR_UNEXPECTED;

Comment 13

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6baf3d309647

Comment 14

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1256473 - Cast values to avoid C4838 on VS2015; r?mayhemer

Review commit: https://reviewboard.mozilla.org/r/41519/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41519/

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8733019 [details]
MozReview Request: Bug 1256473 - Cast values to avoid C4838 on VS2015; r?mayhemer

https://reviewboard.mozilla.org/r/41519/#review38183

::: netwerk/base/nsSocketTransport2.cpp:3028
(Diff revision 1)
>      // Windows allows idle time and retry interval to be set; NOT ping count.
>      struct tcp_keepalive keepalive_vals = {
> -        (int)aEnabled,
> +        (u_long)aEnabled,
>          // Windows uses msec.
> -        aIdleTime * 1000,
> -        aRetryInterval * 1000
> +        (u_long)(aIdleTime * 1000),
> +        (u_long)(aRetryInterval * 1000)

would u_long(aIdleTime) * 1000UL be better?  

it won't help much with overflows but will be IMO a bit more consistent at least regarding signing.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8733019 [details]
MozReview Request: Bug 1256473 - Cast values to avoid C4838 on VS2015; r?mayhemer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41519/diff/1-2/

Comment 17

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/41519/#review38183

> would u_long(aIdleTime) * 1000UL be better?  
> 
> it won't help much with overflows but will be IMO a bit more consistent at least regarding signing.

Will land with the UL.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e0c5caadbc

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8e0c5caadbc