Issue 416002: Android ucontext_t handling refinement. (Closed)
|
Created:
11 years, 9 months ago by digit Modified:
11 years, 9 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAndroid ucontext_t handling refinement. This is a forward-compatible patch to prepare for future versions of the Android C library headers that define ucontext_t. When this patch is applied, Google Breakpad will build and work properly against any version of the NDK or platform headers. See http://code.google.com/p/android/issues/detail?id=34784 for more context. Committed: https://code.google.com/p/google-breakpad/source/detail?r=1000 Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #MessagesTotal messages: 10
https://breakpad.appspot.com/416002/diff/1/src/client/linux/android_ucontext.h File src/client/linux/android_ucontext.h (right): https://breakpad.appspot.com/416002/diff/1/src/client/linux/android_ucontext.... src/client/linux/android_ucontext.h:48: #include <asm/sigcontext.h> /* Ensure 'struct sigcontext' is defined. */ nit: C++ comments are used elsewhere in the file. Also, I don't think I've seen inline comments after a #include in the source anywhere, seems to be unusual. https://breakpad.appspot.com/416002/diff/1/src/client/linux/android_ucontext.... src/client/linux/android_ucontext.h:60: Is it worth defining the i386 structs here, or can we just punt and say "use a newer NDK"? I think Android-x86 is uncommon enough to make that reasonable.
Sign in to reply to this message.
https://breakpad.appspot.com/416002/diff/1/src/client/linux/android_ucontext.h File src/client/linux/android_ucontext.h (right): https://breakpad.appspot.com/416002/diff/1/src/client/linux/android_ucontext.... src/client/linux/android_ucontext.h:48: #include <asm/sigcontext.h> /* Ensure 'struct sigcontext' is defined. */ Sure, I'll fix this. https://breakpad.appspot.com/416002/diff/1/src/client/linux/android_ucontext.... src/client/linux/android_ucontext.h:60: On 2012/07/18 13:59:34, Ted Mielczarek wrote: > Is it worth defining the i386 structs here, or can we just punt and say "use a > newer NDK"? I think Android-x86 is uncommon enough to make that reasonable. I'm currently working on fixing the Android x86 Google Breakpad build too. Having the declarations here is a first step towards this. An NDK with the right header is not available yet, and probably a few months away, while my team is currently working to make Chromium on Android x86 a viable project :) For the record, Android x86-based devices are already sold and in the hands of real users (plus, application developers are already reporting these in our support forums). Note that "www.android-x86.org" is a completely different project: it is a fork of the open-source Android source tree that has nothing to do with Google / official Android development (though they do submit fixes from time to time, they don't seem to want to merge back to upstream). I know that all of this is a bit confusing, but in short, Android on x86 is a reality, even if minor compared to ARM. Next in line is MIPS, which is still submitting patches to the platform as we speak, but strives towards the same goal (not a concern for Breakpad which has zero support for MIPS at the moment, as far as I understand).
Sign in to reply to this message.
On Mon, Jul 30, 2012 at 7:07 AM, <digit@chromium.org> wrote: > I'm currently working on fixing the Android x86 Google Breakpad build > too. Having the declarations here is a first step towards this. An NDK > with the right header is not available yet, and probably a few months > away, while my team is currently working to make Chromium on Android x86 > a viable project :) Ah, okay, I wasn't sure if there was an NDK in existence yet that was usable here. In that case I agree with these changes. > For the record, Android x86-based devices are already sold and in the > hands of real users (plus, application developers are already reporting > these in our support forums). I know, I put up a test build of Firefox for Android/x86 and was surprised at the response.
Sign in to reply to this message.
https://breakpad.appspot.com/416002/diff/6001/src/client/linux/android_uconte... File src/client/linux/android_ucontext.h (right): https://breakpad.appspot.com/416002/diff/6001/src/client/linux/android_uconte... src/client/linux/android_ucontext.h:57: // Other fields are not used by Google Breakpad. Don't define them. Well, as long as nobody depends on an array of ucontexts in memory, this is fine. Maybe nobody can depend on it because the size of the struct changes with different NDK versions? Then this is definitely fine. https://breakpad.appspot.com/416002/diff/6001/src/client/linux/android_uconte... src/client/linux/android_ucontext.h:122: #elif defined(__mips__) Blank line after this, like you did with the other CPUs. https://breakpad.appspot.com/416002/diff/6001/src/client/linux/android_uconte... src/client/linux/android_ucontext.h:157: #endif // !__BIONIC_HAVE_UCONTEX_T __BIONIC_HAVE_STRUCT_SIGCONTEXT?
Sign in to reply to this message.
https://breakpad.appspot.com/416002/diff/6001/src/client/linux/android_uconte... File src/client/linux/android_ucontext.h (right): https://breakpad.appspot.com/416002/diff/6001/src/client/linux/android_uconte... src/client/linux/android_ucontext.h:57: // Other fields are not used by Google Breakpad. Don't define them. On 2012/08/01 14:42:41, Mark Mentovai wrote: > Well, as long as nobody depends on an array of ucontexts in memory, this is > fine. > > Maybe nobody can depend on it because the size of the struct changes with > different NDK versions? Then this is definitely fine. It's a bit more complicated than that. In a nutshell the size of the remaining fields hasn't be defined yet. The fields after are a signal mask and some data storage for the state of FPU registers for getcontext()/setcontext(). This means that both the kernel and the C library disagree on its layout. When used inside a signal handler, the mcontext_t.fpregs field points to FP state stored in the signal stack. The exact layout depends on the kernel configuration, and there is no portable way to access this directly (instead, programs like gdbserver use a ptrace(PTRACE_GETVFPREGS,...) call to get them). When used with getcontext()/setcontext(), the layout depends on the implementation of these functions from the C library. For Android, it has not been defined yet, and I'm not sure these will be ever implemented (they are obsoleted by Posix, and implementations are notoriously buggy, even GLibc on ARM and x86, when it comes to saving FPU state on modern processors). Hence my desire to keep this file as simple as possible, since the rest is not used by Breakpad. If you wish so, I can add the full stuff described in https://android-review.googlesource.com/38875 Note that the only place where Breakpad uses getcontext() is in the Linux version of ExceptionHandler::WriteMiniDump() which simply returns false on ARM. The non-ARM implementation seems to use the ucontext_t/getcontext() to get the state of the floating-point registers. Unfortunately, there is no standard way to get them, even on ARM GLibc which defines fpregset_t as an empty struct. This means that it is doubtful that Breakpad is going to ever use getcontext() on ARM in the future. For reference, some of the gory details are at: https://android-review.googlesource.com/#/c/38875/2/libc/arch-arm/include/mac... Hope this helps. https://breakpad.appspot.com/416002/diff/6001/src/client/linux/android_uconte... src/client/linux/android_ucontext.h:122: #elif defined(__mips__) will do
Sign in to reply to this message.
Thanks for the explanation. Keeping the structs simple sounds like the right call. LGTM.
Sign in to reply to this message.
Sorry, I had forgotten to change __BIONIC_HAVE_STRUCT_SIGCONTEXT into __BIONIC_HAVE_UCONTEXT_T. The code compiles ok in both cases, but the second symbol makes more sense.
Sign in to reply to this message.
lgtm https://breakpad.appspot.com/416002/diff/14001/src/client/linux/android_ucont... File src/client/linux/android_ucontext.h (right): https://breakpad.appspot.com/416002/diff/14001/src/client/linux/android_ucont... src/client/linux/android_ucontext.h:158: #endif // !__BIONIC_HAVE_UCONTEX_T This is still missing a T on the end of UCONTEXT.
Sign in to reply to this message.
https://breakpad.appspot.com/416002/diff/14001/src/client/linux/android_ucont... File src/client/linux/android_ucontext.h (right): https://breakpad.appspot.com/416002/diff/14001/src/client/linux/android_ucont... src/client/linux/android_ucontext.h:158: #endif // !__BIONIC_HAVE_UCONTEX_T Oh this, I thought you were commenting on the unbalanced macro names in your previous comment. I'll fix this once I've solved my password issues. Sorry about that.
Sign in to reply to this message.
|
