1089931 - Breakpad build broken, ucontext related.

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Douglas Crosher [:dougc]]

Seeing build failures related to breakpad, using gcc 4.9 and ndk 10c. They appear to be related to ucontext. Older versions of bionic do not define sys/ucontext.h and there is some code in breakpad to work around this, but it looks suspect.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Fix some Android ucontext include file build issues.

The code in toolkit/crashreporter/google-breakpad/src/common/android/include/ucontext.h looks suspect. The Android NDK does not appear to define __BIONIC_UCONTEXT_H, but rather __BIONIC_HAVE_UCONTEXT_T.

Also it appears that the include paths have overlapping files, ucontext.h and sys/ucontext.h, and how can this work?

After a hack patch for the above, another issues was exposed in xpcom/threads/ThreadStackHelper.cpp.

Comment 2

[(not currently active) Ted Mielczarek]

This was fixed upstream here:
https://code.google.com/p/google-breakpad/source/detail?r=1396

Unfortunately that change makes the build *require* NDK r10c. My recommendation would be to either use an older NDK or --disable-crashreporter until such time as we pick up that patch and require r10c.

Comment 3

[Douglas Crosher [:dougc]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> This was fixed upstream here:
> https://code.google.com/p/google-breakpad/source/detail?r=1396
> 
> Unfortunately that change makes the build *require* NDK r10c. My
> recommendation would be to either use an older NDK or
> --disable-crashreporter until such time as we pick up that patch and require
> r10c.

Ok, thanks. I are compiling with --disable-crashreporter, and trying to deal with the build failures.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8512314 [details] [diff] [review]
Fix some Android ucontext include file build issues.

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

::: toolkit/crashreporter/google-breakpad/src/common/android/include/ucontext.h
@@ +37,3 @@
>  #else
> +# include <sys/ucontext.h>
> +#endif  // __BIONIC_HAVE_UCONTEXT_T

You're including the same file in both branches, and including all the above in both cases, which is not what you'd want. What you'd want is to have this file be a dummy wrapper for sys/ucontext.h when __BIONIC_HAVE_UCONTEXT_T is defined (which, you'll have to note, only is defined when signal.h is included), or define breakpad_getcontext, etc. when __BIONIC_HAVE_UCONTEXT_T is not defined.

ThreadStackHelper.cpp shouldn't need a change.

Comment 6

[Douglas Crosher [:dougc]]

Attachment: Android NDK r10c includes ucontext.h

Thank you for the feedback. This patch passes a few different build variations locally. Try run to see how it goes with the default build environment: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb8e48483634

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8515429 [details] [diff] [review]
Android NDK r10c includes ucontext.h

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

::: toolkit/crashreporter/google-breakpad/src/common/android/include/ucontext.h
@@ +38,3 @@
>  #else
> +# include <sys/ucontext.h>
> +#endif  // __BIONIC_UCONTEXT_H

I think that #endif should stay where it currently is.

Comment 8

[Douglas Crosher [:dougc]]

(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 8515429 [details] [diff] [review]
...
> > +# include <sys/ucontext.h>
> > +#endif  // __BIONIC_UCONTEXT_H
> 
> I think that #endif should stay where it currently is.

Won't getcontext(x) need to be defined in both cases?

Comment 9

[Mike Hommey [:glandium]]

(In reply to Douglas Crosher [:dougc] from comment #8)
> Won't getcontext(x) need to be defined in both cases?

Doesn't the ndk ucontext.h define it?

Comment 10

[Douglas Crosher [:dougc]]

(In reply to Mike Hommey [:glandium] from comment #9)
> (In reply to Douglas Crosher [:dougc] from comment #8)
> > Won't getcontext(x) need to be defined in both cases?
> 
> Doesn't the ndk ucontext.h define it?

No. A search for getcontext does not turn up anything in the NDK include files.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8515429 [details] [diff] [review]
Android NDK r10c includes ucontext.h

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

sigh. broken bionic headers are broken.

Comment 12

[Douglas Crosher [:dougc]]

Thank you for the review. Requesting checkin. There is a try run in comment 6.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/6c11f40538b4

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c11f40538b4