1181026 - (32-bit compilation) ipc/chromium/src/third_party/libevent/buffer.c:2518:18: warning: comparison is always false due to limited range of data type [-Wtype-limits]

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[ISHIKAWA, Chiaki]

(32-bit compilation) ipc/chromium/src/third_party/libevent/buffer.c:2518:18: warning: comparison is always false due to limited range of data type [-Wtype-limits]

I have compiled C-C TB under 32-bit linux and found the disturbing
warning messages at compilation time. 

The system is Debian GNU/linux 32-bit:
uname -a
Linux debian-vbox-ci 3.2.0-4-686-pae #1 SMP Debian 3.2.68-1+deb7u1 i686 GNU/Linux

According to the mailing post to inquire whether this problem may have
to be dealt with the upstream, 
Mike Hommey suggested

"It's a mozilla only problem in event-config.h for linux and mac. Please file a bug." and so here is a bug report.
(
https://groups.google.com/forum/?_escaped_fragment_=topic/mozilla.dev.builds/N5-eitNhe9g#!topic/mozilla.dev.builds/N5-eitNhe9g )


- Analysis of the warning (excerpt from the log is at the end of this post).

I think due to the failure of the comparison because of the size
mismatch of literal constant (EV_SSIZE_MAX and friends) and declared
variable (or structure member), some run-time tests are ineffective
and some error conditions are not handled correctly.

From my observation below, I think EV_SSIZE_MAX and EV_SIZE_MAX ought
to be defined as 32-bit under 32-bit linux: the lietrals seem to be
defined as 64-bit entity today under 32-bit linux.

I understand that for official distribution,
mozilla compilation farm seems to use cross compiling from the 64-bit
linux to produce 32-bit linux version of firefox and thunderbird.
If so, then maybe for the official version, it may happen to pick up
the right size (accidentally, by mistake, or intentionally).
I suspect there is something incorrect in the headers.

--- Source Files with the problems:

The problem occurs in mozilla/ipc/chromium/src/third_party/libevent
directory.

(This seems to be a mozilla-only bug as noted by Mike Hommey.)

--- background:

For background information, macro EV_SSIZE_MAX is defined in
http://mxr.mozilla.org/comm-central/source/mozilla/ipc/chromium/src/third_party/libevent/include/event2/util.h#245 
as follows:
presumably on Debian GNU/linux (32-bit) , it probably is defined
as EV_UINT64_MAX (?)

237 #if _EVENT_SIZEOF_SIZE_T == 8
238 #define EV_SIZE_MAX EV_UINT64_MAX
239 #define EV_SSIZE_MAX EV_INT64_MAX
240 #elif _EVENT_SIZEOF_SIZE_T == 4
241 #define EV_SIZE_MAX EV_UINT32_MAX
242 #define EV_SSIZE_MAX EV_INT32_MAX
243 #elif defined(_EVENT_IN_DOXYGEN)
244 #define EV_SIZE_MAX ...
245 #define EV_SSIZE_MAX ...
246 #else
247 #error "No way to define SIZE_MAX"
248 #endif

EV_RATE_LIMIT_MAX is defined as the same as EV_SSIZE_MAX.

http://mxr.mozilla.org/comm-central/source/mozilla/ipc/chromium/src/third_party/libevent/include/event2/bufferevent.h#613

612 /** Maximum configurable rate- or burst-limit. */
613 #define EV_RATE_LIMIT_MAX EV_SSIZE_MAX

On my 32-bit Debian GNU/linux, the next code prints out the
outout that follows: I compiled this with simple gcc hello.c
--- hello.c ---
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>

main()
{
  printf("off_t is %d octets\n", sizeof (off_t));
  printf("size_t is %d octets\n", sizeof (size_t));  
}
--- end ----

This code after compilation prints out the following:
off_t is 4 octets
size_t is 4 octets

( Hmm, I thought I could use more than 4GB of a single file, but maybe
not under 32-bit linux? [more like the old ext3 limit] 
The file system as a whole can certainly be larger than 4GB. But I digress.)

--- The disturbing compilation log

Here is an excerpt from the compilation log.


--- begin quote of log with a few comments thrown in.---
...
Unified_c_ipc_chromium0.o
Unified_c_ipc_chromium1.o
nsCommandLine.o
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:2:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/buffer.c: In function 'evbuffer_search_range':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/buffer.c:2518:18: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  if (!len || len > EV_SSIZE_MAX)

  [CI's comment: len is defined as |size_t len| to the enclosing
  function. It looks "size_t" is defined to be 32-bit entity on my
  system.]
  
                  ^
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:20:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c: In function 'ev_token_bucket_cfg_new':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c:156:16: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  if (read_rate > EV_RATE_LIMIT_MAX ||
                ^
  [CI's comment: this warning and the following three warnings are
   against this code in
   
http://mxr.mozilla.org/comm-central/source/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c#158   


156 	if (read_rate > EV_RATE_LIMIT_MAX ||
157 	    write_rate > EV_RATE_LIMIT_MAX ||
158 	    read_burst > EV_RATE_LIMIT_MAX ||
159 	    write_burst > EV_RATE_LIMIT_MAX)
160 		return NULL;

|read_rate|, |write_rate|, |read_burst|, |write_burst| are |size_t| arguments
to the enclosing function.

/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c:157:17: warning: comparison is always false due to limited range of data type [-Wtype-limits]
      write_rate > EV_RATE_LIMIT_MAX ||
                 ^
	[CI comment. "is always false" does not sound encouraging. ]

/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c:158:17: warning: comparison is always false due to limited range of data type [-Wtype-limits]
      read_burst > EV_RATE_LIMIT_MAX ||
                 ^
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c:159:18: warning: comparison is always false due to limited range of data type [-Wtype-limits]
      write_burst > EV_RATE_LIMIT_MAX)
                  ^
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:20:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c: In function 'bufferevent_rate_limit_group_set_min_share':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c:704:12: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  if (share > EV_SSIZE_MAX)
            ^
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:20:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c: In function 'bufferevent_get_read_limit':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c:828:7: warning: overflow in implicit constant conversion [-Woverflow]
   r = EV_SSIZE_MAX;
       ^

   [CI comment. "overflow" is bad, too.]

/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c: In function 'bufferevent_get_write_limit':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/bufferevent_ratelim.c:847:7: warning: overflow in implicit constant conversion [-Woverflow]
   r = EV_SSIZE_MAX;
       ^
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:92:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/evutil.c: In function 'evutil_read_file':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/evutil.c:149:17: warning: comparison is always false due to limited range of data type [-Wtype-limits]
      st.st_size > EV_SSIZE_MAX-1 ) {
                 ^
      [CI's comment: this message bothers me great deal, and motivated
      me to post the inquiry to the mailing list.

      st_size ought to be large enough to cover the largest file offset.

      The warning is is from the following code. As you can see, |st|
      is a struct that is passed to |fstat| and so |st_size| can hold the
      largest file size that is limited by the file system under a
      particular linux installation.

148 	if (fstat(fd, &st) || st.st_size < 0 ||
149 	    st.st_size > EV_SSIZE_MAX-1 ) {
150 		close(fd);
151 		return -2;

So what is the type of |st_size|?
From the linux fstat man page:
           struct stat {
               dev_t     st_dev;         /* ID of device containing file */
               ino_t     st_ino;         /* inode number */
               mode_t    st_mode;        /* protection */
               nlink_t   st_nlink;       /* number of hard links */
               uid_t     st_uid;         /* user ID of owner */
               gid_t     st_gid;         /* group ID of owner */
               dev_t     st_rdev;        /* device ID (if special file) */
               off_t     st_size;        /* total size, in bytes */
               blksize_t st_blksize;     /* blocksize for filesystem I/O */
               blkcnt_t  st_blocks;      /* number of 512B blocks allocated */

               /* Since Linux 2.6, the kernel supports nanosecond
                  precision for the following timestamp fields.
                  For the details before Linux 2.6, see NOTES. */

               struct timespec st_atim;  /* time of last access */
               struct timespec st_mtim;  /* time of last modification */
               struct timespec st_ctim;  /* time of last status change */

           #define st_atime st_atim.tv_sec      /* Backward compatibility */
           #define st_mtime st_mtim.tv_sec
           #define st_ctime st_ctim.tv_sec
           };

	   [CI's comment: As we can see from my comment near the beginning of this
	   post, on this system, |off_t| is 32-bit.
	   So from the observation here and and other info,
	   I think |EV_SSIZE_MAX| ought to be  32-bit entity
	   on this system, but it seems somehow it ended up being defined as
	   64-bit entity(?).]

In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:110:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c: In function 'evhttp_htmlescape':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c:269:20: warning: comparison of promoted ~unsigned with unsigned [-Wsign-compare]
   if (replace_size > EV_SIZE_MAX - new_size) {
                    ^
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c:276:15: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  if (new_size == EV_SIZE_MAX)
               ^

	       [CI comment: this always false is bad, too. ]

/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c: In function 'evhttp_connection_set_max_headers_size':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c:631:29: warning: large integer implicitly truncated to unsigned type [-Woverflow]
   evcon->max_headers_size = EV_SIZE_MAX;
                             ^
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:110:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c: In function 'evhttp_handle_chunked_read':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c:857:14: warning: comparison is always false due to limited range of data type [-Wtype-limits]
   if (buflen > EV_SSIZE_MAX) {
              ^
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:110:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c: In function 'evhttp_connection_base_new':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c:2087:28: warning: large integer implicitly truncated to unsigned type [-Woverflow]
  evcon->max_headers_size = EV_SIZE_MAX;
                            ^
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:110:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c: In function 'evhttp_new_object':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c:3200:36: warning: overflow in implicit constant conversion [-Woverflow]
  evhttp_set_max_headers_size(http, EV_SIZE_MAX);
                                    ^
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c:3201:33: warning: overflow in implicit constant conversion [-Woverflow]
  evhttp_set_max_body_size(http, EV_SIZE_MAX);
                                 ^
In file included from /home/ishikawa/objdir-tb3/ipc/chromium/Unified_c_ipc_chromium0.c:110:0:
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c: In function 'evhttp_set_max_headers_size':
/new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/ipc/chromium/src/third_party/libevent/http.c:3377:36: warning: large integer implicitly truncated to unsigned type [-Woverflow]
   http->default_max_headers_size = EV_SIZE_MAX;
                                    ^
atomicops_internals_x86_gcc.o
idle_timer_none.o
libtoolkit_components_commandlines.a.desc
message_pump_glib.o
process_util_linux.o
time_posix.o
Unified_cpp_ipc_chromium0.o
Unified_cpp_ipc_chromium1.o
Unified_cpp_ipc_chromium2.o
Unified_cpp_ipc_chromium3.o
...
--- end quote of log


TIA

Comment 1

[Mike Hommey [:glandium]]

The problem is that _EVENT_SIZEOF_SIZE_T is hard set to 8 on linux and mac, in resp. ipc/chromium/src/third_party/libevent/linux/event2/event-config.h and ipc/chromium/src/third_party/libevent/mac/event2/event-config.h.

Other hard coded values that are probably problematic as well:
_EVENT_SIZEOF_LONG
_EVENT_SIZEOF_PTHREAD_T
_EVENT_SIZEOF_VOID_P

Comment 2

[Mike Hommey [:glandium]]

(and to be clear, those files don't come from upstream, they come from manually running configure for each platform and checking them in our source tree)

Comment 3

[Nicholas Nethercote [inactive]]

I have a draft patch that adds an event-config.h file for linux32. It seems to make the try server happy. I'll do bug 1206558 first, though.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Create an event-config.h file for 32-bit Linux builds

I feel like the |CONFIG['CPU_ARCH'] == 'x86'| test is missing some cases, but I
couldn't work out what else to put there.

Note that this patch applies on top of the patches in bug 1206558.

Comment 5

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b60cdbe65267

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8663556 [details] [diff] [review]
Create an event-config.h file for 32-bit Linux builds

Review of attachment 8663556 [details] [diff] [review]:
-----------------------------------------------------------------

Please update ipc/chromium/src/third_party/libevent/README.mozilla too, and please do the same on mac.

Although, it /might/ be a good idea to just use #ifdefs in the existing event-config.h files.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Wrap long lines in libevent's README.mozilla file

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Clean up libevent patch handling

Add a missing one to the docs, and move them from their current two locations
into a new patches/ directory.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Fix libevent constants for 32-bit Linux/Mac/BSD builds

The patch also adds the CHECK_EVENT_SIZEOF macro which checks that the
_EVENT_SIZEOF_* constants have the right values.

Comment 10

[Nicholas Nethercote [inactive]]

Try looks good for the new patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=558df5d960f0

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8664052 [details] [diff] [review]
(part 1) - Wrap long lines in libevent's README.mozilla file

Review of attachment 8664052 [details] [diff] [review]:
-----------------------------------------------------------------

Nit: You're also adding dashes before items, you might as well say you're reformatting the file instead of wrapping long lines.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8664053 [details] [diff] [review]
(part 2) - Clean up libevent patch handling

Review of attachment 8664053 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/third_party/libevent/README.mozilla
@@ +30,5 @@
>  
> +- "dont-use-issetugid-on-android.patch". This fixes the build on Android L
> +  preview.
> +
> +- "avoid-empty-sighandler.patch". This fixes some OS X crashes.

I assume you checked doing the above leads to the same as what is in the tree?

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8664054 [details] [diff] [review]
(part 3) - Fix libevent constants for 32-bit Linux/Mac/BSD builds

Review of attachment 8664054 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/base/message_pump_libevent.cc
@@ +20,5 @@
>  
> +// This macro checks that the _EVENT_SIZEOF_* constants defined in
> +// ipc/chromiume/src/third_party/<platform>/event2/event-config.h are correct.
> +#define CHECK_EVENT_SIZEOF(TYPE, type) \
> +    static_assert(_EVENT_SIZEOF_##TYPE == sizeof(type), \

+1

::: ipc/chromium/src/third_party/libevent/README.mozilla
@@ +4,5 @@
>  1. Add the following files:
>  
>  - linux/event2/event-config.h
>  - mac/event2/event-config.h
> +- bsd/event2/event-config.h

Thanks for finding this one :)

Comment 14

[Nicholas Nethercote [inactive]]

> > +- "avoid-empty-sighandler.patch". This fixes some OS X crashes.
> 
> I assume you checked doing the above leads to the same as what is in the
> tree?

I checked that avoid-empty-sighandler.patch can be applied in reverse.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dae845bd6cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd95cb81260
https://hg.mozilla.org/integration/mozilla-inbound/rev/4051dc52b0c5

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3dae845bd6cb
https://hg.mozilla.org/mozilla-central/rev/9cd95cb81260
https://hg.mozilla.org/mozilla-central/rev/4051dc52b0c5