1388981 - error: static assertion failed: bad EVENT__SIZEOF_OFF_T on 32-bits linux with system libevent

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Mike Hommey [:glandium]]

Depending how the system libevent is built, EVENT_SIZEOF_OFF_T can mismatch, because off_t can actually be 64-bits on 32-bits platforms (when _FILE_OFFSET_BITS is set to 64). This leads the build to fail with the error message in the summary.

Comment 1

[Mike Hommey [:glandium]]

Note the same error happens when building firefox with CPPFLAGS=-D_FILE_OFFSET_BITS=64.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1388981 - Fixup EVENT__SIZEOF_OFF_T.

There are two problems related with EVENT__SIZEOF_OFF_T:
- When building Firefox with -D_FILE_OFFSET_BITS=64, off_t is 64 bits,
  but the in-tree event-config.h still defines EVENT__SIZEOF_OFF_T to 4.
- When building Firefox *without* -D_FILE_OFFSET_BITS=64 (the default)
  against a system libevent that was built with -D_FILE_OFFSET_BITS=64,
  its event-config.h defines EVENT__SIZEOF_OFF_T to 8, which then
  doesn't match off_t size.

For the latter, libevent actually defines its own off_t type, that
callers are supposed to use instead of off_t. So that's what our
static_assert should be checking.

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

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8895666 [details]
Bug 1388981 - Fixup EVENT__SIZEOF_OFF_T.

https://reviewboard.mozilla.org/r/166940/#review172480

Looks good.

::: ipc/chromium/src/base/message_pump_libevent.cc:42
(Diff revision 1)
>  CHECK_EVENT_SIZEOF(LONG,      long);
>  CHECK_EVENT_SIZEOF(LONG_LONG, long long);
> -CHECK_EVENT_SIZEOF(OFF_T,     off_t);
> +CHECK_EVENT_SIZEOF(OFF_T,     ev_off_t);
>  CHECK_EVENT_SIZEOF(PTHREAD_T, pthread_t);
>  CHECK_EVENT_SIZEOF(SHORT,     short);
>  CHECK_EVENT_SIZEOF(SIZE_T,    size_t);

I wonder if this should be `ev_ssize_t`, to match how the macro is used: https://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/ipc/chromium/src/third_party/libevent/evbuffer-internal.h#158

But I suppose it's unlikely for `ev_ssize_t` to ever be different from the real `ssize_t`.

Comment 4

[Mike Hommey [:glandium]]

ev_ssize_t is just an alias for ssize_t per https://searchfox.org/mozilla-central/source/ipc/chromium/src/third_party/libevent/include/event2/util.h#203

As for the other branch there, EVENT__ssize_t is based on the result of those CMake checks:
https://searchfox.org/mozilla-central/source/ipc/chromium/src/third_party/libevent/CMakeLists.txt#444

So the only intent here is to support old compilers that don't have the real type proper, and compilers that do would have a definition that matches anyways, except in that corner case: https://searchfox.org/mozilla-central/source/ipc/chromium/src/third_party/libevent/CMakeLists.txt#450 but we're talking about a rare odd case.

All in all, I don't think ev_ssize_t matters.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15517c5a5d37
Fixup EVENT__SIZEOF_OFF_T. r=jld

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/15517c5a5d37