Bug #5833

[mingw] trivial patch for thread.c build warning

Added by Jon Forums about 3 years ago. Updated about 3 years ago.

[ruby-core:41870]
Status:Closed
Priority:Normal
Assignee:Akira Tanaka
ruby -v:- Backport:

Description

When building with MinGW (not Windows SDK) on Win7 I get the following:

compiling ../../../../Users/Jon/Documents/RubyDev/ruby-git/thread.c
../../../../Users/Jon/Documents/RubyDev/ruby-git/thread.c: In function 'rb_fd_rcopy':
../../../../Users/Jon/Documents/RubyDev/ruby-git/thread.c:2471:33: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Below is a trivial patch that fixes the warning on MinGW. The patch successfully builds and tests with MinGW and Windows SDK on Win7 32bit. When building on Arch Linux, the patched build gives the same make test && make test-all results (6 errors, 45 skips) as an unpatched build. In all other uses in thread.c (except for rb_fd_rcopy) the rb_fd_max macro generates a size_t object. From what I can tell, use cases are all unsigned int's.

diff --git a/thread.c b/thread.c
index d9fe5506..0b48061 100644
--- a/thread.c
+++ b/thread.c
@@ -2463,7 +2463,7 @@ rb_fd_init_copy(rb_fdset_t *dst, rb_fdset_t *src)
static void
rb_fd_rcopy(fd_set *dst, rb_fdset_t *src)
{
- int max = rb_fd_max(src);
+ size_t max = rb_fd_max(src);

 /* we assume src is the result of select() with dst, so dst should be
  * larger or equal than src. */

History

#1 Updated by Motohiro KOSAKI about 3 years ago

  • ruby -v changed from ruby 2.0.0dev (2011-12-31 trunk 34165) [i386-mingw32] to -

Below is a trivial patch that fixes the warning on MinGW. The patch successfully builds and tests with MinGW and Windows SDK on Win7 32bit. When building on Arch Linux, the patched build gives the same make test && make test-all results (6 errors, 45 skips) as an unpatched build. In all other uses in thread.c (except for rb_fd_rcopy) the rb_fd_max macro generates a size_t object. From what I can tell, use cases are all unsigned int's.

diff --git a/thread.c b/thread.c
index d9fe5506..0b48061 100644
--- a/thread.c
+++ b/thread.c
@@ -2463,7 +2463,7 @@ rb_fd_init_copy(rb_fdset_t *dst, rb_fdset_t *src)
 static void
 rb_fd_rcopy(fd_set *dst, rb_fdset_t *src)
 {
-    int max = rb_fd_max(src);
+    size_t max = rb_fd_max(src);

    /* we assume src is the result of select() with dst, so dst should be
     * larger or equal than src. */

I think rb_fd_max() should return int. Is there any possibility that
fdset->fd_count overflow signed int?

Moreover Windows fd_set::fd_count has u_int type if a documentation is
correct (I saw http://msdn.microsoft.com/en-us/library/windows/desktop/ms737873(v=vs.85).aspx)
and size_t is not an alias of u_int.

#2 Updated by Jon Forums about 3 years ago

Redmine appears to have chopped off your feedback and questions...inserting your questions and my answers.

I think rb_fd_max() should return int. Is there any possibility that
fdset->fd_count overflow signed int?

In real-world usage, I don't know with 100% certainty.

But since the following returns a u_int on Windows

https://github.com/ruby/ruby/blob/trunk/include/ruby/intern.h#L298

and this returns FD_SETSIZE (usually #defined)

https://github.com/ruby/ruby/blob/trunk/include/ruby/intern.h#L314

overflow of signed int might be a possibility.

Why do you think rb_fd_max should return int rather than unsigned int? Is it because of this:

https://github.com/ruby/ruby/blob/trunk/include/ruby/intern.h#L260

Do you think there is a possibility of returning a negative int (maybe an extension?) in a valid real-world scenario?

Moreover Windows fd_set::fd_count has u_int type if a documentation is
correct (I saw http://msdn.microsoft.com/en-us/library/windows/desktop/ms737873(v=vs.85).aspx)
and size_t is not an alias of u_int.

For 32bit mingw64, headers appear to have typedef unsigned int size_t like lines 380-388 of:

http://mingw-w64.svn.sourceforge.net/viewvc/mingw-w64/tags/v2.0.1/mingw-w64-headers/crt/_mingw.h.in?revision=4710&view=markup

For 32bit mingw64, fdset->fd_count type is u_int:

http://mingw-w64.svn.sourceforge.net/viewvc/mingw-w64/tags/v2.0.1/mingw-w64-headers/include/psdk_inc/_fd_types.h?revision=4710&view=markup

with typedef unsigned int u_int like:

http://mingw-w64.svn.sourceforge.net/viewvc/mingw-w64/tags/v2.0.1/mingw-w64-headers/crt/_bsd_types.h?revision=4710&view=markup

And I see similar typedefs when searching my local mingw.org headers for u_int and size_t.

In my Windows 7 SDK I see the following in io.h and crtdefs.h:

#ifndef SIZE_T_DEFINED
#ifdef _WIN64
typedef unsigned __int64 size_t;
#else
typedef _W64 unsigned int size
t;
#endif
#define _SIZE_T_DEFINED
#endif

in which _W64 is defined like i:

#if !defined(W64)
#if !defined(
midl) && (defined(_X86) || defined(_M_IX86))
#define _W64 __w64
#else
#define _W64
#endif
#endif

#3 Updated by Koichi Sasada about 3 years ago

  • Assignee set to Akira Tanaka

#4 Updated by Nobuyoshi Nakada about 3 years ago

  • Status changed from Open to Closed

Also available in: Atom PDF