1385667 - Firefox fails to build with recent GLIBC: error: field 'context' has incomplete type 'google_breakpad::ucontext'

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Robert-André Mauchin]

When building firefox with a recent GLIBC, the follwoing error occurs:


>make[5]: Entering directory '/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/toolkit/crashreporter'
>mkdir -p '.deps/'
>/usr/bin/g++ -std=gnu++11 -o Unified_cpp_crashreporter0.o -c -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/dist/stl_wrappers -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/dist/system_wrappers -include /builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DOS_POSIX=1 -DOS_LINUX=1 -DUNICODE -D_UNICODE -DNO_STABS_SUPPORT -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/toolkit/crashreporter -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/toolkit/crashreporter -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/ipc/ipdl/_ipdlheaders -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/ipc/chromium/src -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/ipc/glue -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/toolkit/crashreporter/google-breakpad/src -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/toolkit/crashreporter/breakpad-client -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/toolkit/crashreporter/google-breakpad/src -I/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/dist/include  -I/usr/include/nspr4 -I/usr/include/nss3       -fPIC  -DMOZILLA_CLIENT -include /builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/mozilla-config.h -MD -MP -MF .deps/Unified_cpp_crashreporter0.o.pp  -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++14-compat -Wc++1z-compat -Wimplicit-fallthrough -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -Wformat-overflow=2 -O2 -g -pipe -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -Wformat-security -Wformat -Werror=format-security -fno-delete-null-pointer-checks -fPIC -Wl,-z,relro -Wl,-z,now -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe  -g -g -O2 -fno-omit-frame-pointer   -Wno-unused-local-typedefs -Wno-shadow -Wno-deprecated-declarations -Wno-bool-compare -Wno-unused-but-set-variable -Wno-shadow  /builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/toolkit/crashreporter/Unified_cpp_crashreporter0.cpp
>In file included from /builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/toolkit/crashreporter/nsExceptionHandler.cpp:68:0,
>                 from /builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/toolkit/crashreporter/Unified_cpp_crashreporter0.cpp:11:
>/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/toolkit/crashreporter/breakpad-client/linux/handler/exception_handler.h:194:21: error: field 'context' has incomplete type 'google_breakpad::ucontext'
>     struct ucontext context;
>                     ^~~~~~~
>/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/toolkit/crashreporter/breakpad-client/linux/handler/exception_handler.h:194:12: note: forward declaration of 'struct google_breakpad::ucontext'
>     struct ucontext context;
>            ^~~~~~~~
>make[5]: Leaving directory '/builddir/build/BUILD/gecko-dev-65bbd0525acd0a981c3811b11e23cb057a06373d/objdir/toolkit/crashreporter'


The error occurs because ucontext declared in <sys/ucontext.h> has been recently renamed in recent version of the GLIBC to ucontext_t: see commit https://sourceware.org/git/?p=glibc.git;a=commit;f=sysdeps/i386/sys/ucontext.h;h=251287734e89a52da3db682a8241eb6bccc050c9

Comment 1

[Robert-André Mauchin]

@ted.mielczarek You're the one who forked breakpad client into Firefox, could you have a look at this?

Comment 2

[(not currently active) Ted Mielczarek]

It looks like we could probably just change `struct ucontext` to `ucontext_t` there:
https://dxr.mozilla.org/mozilla-central/rev/5322c03f4c8587fe526172d3f87160031faa6d75/toolkit/crashreporter/breakpad-client/linux/handler/exception_handler.h#194

Since the typedef to `ucontext_t` has existed this whole time and still exists.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I hit this today...

Comment 4

[Emilio Cobos Álvarez [:emilio]]

(The triple dots was because I'm writing a patch, didn't intend to be mean btw :P)

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1385667: Use ucontext_t consistently in breakpad-client.

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

Comment 6

[Robert-André Mauchin]

Attachment: Use ucontext or ucontext_t based on GLIBC version

Comment 7

[Robert-André Mauchin]

The patch will work with new version of GLIBC, but what if one tries to build with an older version of GLIBC? It will fail too because ucontext_t is not defined. I patched my build with a lot of #if __GLIBC__ == 2 && __GLIBC_MINOR__ < 26 because I need to build Firefox on platforms with different versions of GLIBC.

I attached the patch I use to show what I mean.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

(In reply to Robert-André Mauchin from comment #7)
> The patch will work with new version of GLIBC, but what if one tries to
> build with an older version of GLIBC? It will fail too because ucontext_t is
> not defined. I patched my build with a lot of #if __GLIBC__ == 2 &&
> __GLIBC_MINOR__ < 26 because I need to build Firefox on platforms with
> different versions of GLIBC.
> 
> I attached the patch I use to show what I mean.

As ted mentioned in comment 2, GLIBC has changed from:

  typedef struct ucontext { ... } ucontext_t;

to:

  typedef struct ucontext_t { ... } ucontext_t;

Thus, my patch, that switches everything to just ucontext_t, should be enough to build with both old and new versions of glibc.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

To prove this builds with older glibc versions, an automation build with this patch:

 * https://treeherder.mozilla.org/#/jobs?repo=try&revision=95eaad1a25c036729b04e15f8b9a10b1d707415a

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8899165 [details]
Bug 1385667: Use ucontext_t consistently in breakpad-client.

https://reviewboard.mozilla.org/r/170444/#review175796

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3970338a22b
Use ucontext_t consistently in breakpad-client. r=ted

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Jan Alexander Steffens [:heftig]]

I still have problems building with glibc 2.26 even after this patch:

> 3:44.64 In file included from /build/firefox/src/mozilla-unified/obj-i686-pc-linux-gnu/toolkit/crashreporter/breakpad-client/linux/Unified_cpp_linux0.cpp:29:0:
> 3:44.64 /build/firefox/src/mozilla-unified/toolkit/crashreporter/breakpad-client/linux/dump_writer_common/ucontext_reader.cc:51:63: error: ‘ucontext’ does not name a type; did you mean ‘ucontext_t’?
> 3:44.64  void UContextReader::FillCPUContext(RawContextCPU *out, const ucontext *uc,
> 3:44.64                                                                ^~~~~~~~
> 3:44.64                                                                ucontext_t
> 3:44.64 /build/firefox/src/mozilla-unified/toolkit/crashreporter/breakpad-client/linux/dump_writer_common/ucontext_reader.cc:51:6: error: prototype for ‘void google_breakpad::UContextReader::FillCPUContext(google_breakpad::RawContextCPU*, const int*, const _libc_fpstate*)’ does not match any in class ‘google_breakpad::UContextReader’
> 3:44.64  void UContextReader::FillCPUContext(RawContextCPU *out, const ucontext *uc,
> 3:44.64       ^~~~~~~~~~~~~~
> 3:44.64 In file included from /build/firefox/src/mozilla-unified/toolkit/crashreporter/breakpad-client/linux/dump_writer_common/ucontext_reader.cc:30:0,
> 3:44.64                  from /build/firefox/src/mozilla-unified/obj-i686-pc-linux-gnu/toolkit/crashreporter/breakpad-client/linux/Unified_cpp_linux0.cpp:29:
> 3:44.64 /build/firefox/src/mozilla-unified/toolkit/crashreporter/breakpad-client/linux/dump_writer_common/ucontext_reader.h:52:15: error: candidate is: static void google_breakpad::UContextReader::FillCPUContext(google_breakpad::RawContextCPU*, const ucontext_t*, const _libc_fpstate*)
> 3:44.64    static void FillCPUContext(RawContextCPU *out, const ucontext_t *uc,
> 3:44.64                ^~~~~~~~~~~~~~

Seems some instances of "const ucontext *uc" were missed?

Comment 14

[Emilio Cobos Álvarez [:emilio]]

(In reply to Jan Alexander Steffens from comment #13)
> Seems some instances of "const ucontext *uc" were missed?

Yup, those are arch-dependent, and I missed them, d'oh. Filed bug 1394149 and posted a patch.

Comment 15

[Ian Neal]

Attachment: breakpad_linux.diff

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fix building with later GLIBC on ESR
User impact if declined: Local builders cannot build with later GLIBC
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Ted, is this something we should uplift to ESR52.6? If yes, could you please r+ the patch. Thanks!

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 8936147 [details] [diff] [review]
breakpad_linux.diff

This is a pretty inoffensive patch. It doesn't actually change anything for our official builds, but if it unbreaks compiling ESR with a newer glibc then it's probably worth landing it just to remove that headache.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8936147 [details] [diff] [review]
breakpad_linux.diff

Thanks Ted. Let's take it then. ESR52+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/57d4fae4dffe

Comment 20

[Mike Hommey [:glandium]]

FWIW, this was fixed in upstream breakpad.
https://github.com/google/breakpad/commit/bddcc58860f522a0d4cbaa7e9d04058caee0db9d