Breakpad build broken, ucontext related.

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

unspecified
mozilla36
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ndk-r10c])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8512314 [details] [diff] [review]
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.
Flags: needinfo?(mh+mozilla)
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.
(Assignee)

Comment 3

4 years ago
(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 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.
Attachment #8512314 - Flags: feedback-
Flags: needinfo?(mh+mozilla)
Duplicate of this bug: 1091053
Whiteboard: [ndk-r10c]
(Assignee)

Comment 6

4 years ago
Created attachment 8515429 [details] [diff] [review]
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
Attachment #8512314 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8515429 - Flags: review?(mh+mozilla)
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.
Attachment #8515429 - Flags: review?(mh+mozilla) → feedback+
(Assignee)

Comment 8

4 years ago
(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?
Flags: needinfo?(mh+mozilla)
(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?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 10

4 years ago
(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 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.
Attachment #8515429 - Flags: feedback+ → review+
(Assignee)

Comment 12

4 years ago
Thank you for the review. Requesting checkin. There is a try run in comment 6.
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c11f40538b4
Assignee: nobody → dtc-moz
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.