Closed Bug 1181026 Opened 11 years ago Closed 10 years ago

(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]

Categories

(Core :: IPC, defect)

Product:
Core
Component:
IPC
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: ishikawa, Assigned: n.nethercote)

References

Details

Attachments

(3 files, 1 obsolete file)

(part 1) - Wrap long lines in libevent's README.mozilla file
2.93 KB, patch
glandium
: review+
Details | Diff | Splinter Review
(part 2) - Clean up libevent patch handling
3.92 KB, patch
glandium
: review+
Details | Diff | Splinter Review
(part 3) - Fix libevent constants for 32-bit Linux/Mac/BSD builds
9.63 KB, patch
glandium
: review+
Details | Diff | Splinter Review
(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
Summary: (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 comp → (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]
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
Component: Event Handling → IPC
(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)
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.
Blocks: 1205942
Depends on: 1206558
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.
Attachment #8663556 - Flags: review?(mh+mozilla)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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.
Attachment #8663556 - Flags: review?(mh+mozilla)
Add a missing one to the docs, and move them from their current two locations into a new patches/ directory.
Attachment #8664053 - Flags: review?(mh+mozilla)
The patch also adds the CHECK_EVENT_SIZEOF macro which checks that the _EVENT_SIZEOF_* constants have the right values.
Attachment #8664054 - Flags: review?(mh+mozilla)
Attachment #8663556 - Attachment is obsolete: true
No longer depends on: 1206558
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.
Attachment #8664052 - Flags: review?(mh+mozilla) → review+
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?
Attachment #8664053 - Flags: review?(mh+mozilla) → review+
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 :)
Attachment #8664054 - Flags: review?(mh+mozilla) → review+
> > +- "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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: