Closed
Bug 708733
Opened 13 years ago
Closed 12 years ago
Profiler tool has ARM specific code that breaks x86 Android
Categories
(Core :: Gecko Profiler, defect, P5)
Tracking
()
RESOLVED
FIXED
People
(Reporter: thomas.g.eaton, Assigned: BenWa)
References
Details
Attachments
(2 files, 1 obsolete file)
1.32 KB,
text/plain
|
Details | |
954 bytes,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1 Build ID: 20110928224508 Steps to reproduce: I tried to build Fennec for x86 Android Actual results: It crashed after running it due to macro assumptions that Android == ARM. Expected results: The macros should also check a platform value to determine whether it's x86 or ARM. Below is a hack that got it working on x86, but isn't a full/correct solution. diff -r aa6ad7ab9b43 tools/profiler/sps/platform.cc --- a/tools/profiler/sps/platform.cc Mon Nov 28 10:34:54 2011 +0000 +++ b/tools/profiler/sps/platform.cc Tue Dec 06 08:33:09 2011 -0800 @@ -132,14 +132,15 @@ static void ProfilerSaveSignalHandler(int signal, siginfo_t* info, void* context) { sActiveSampler->RequestSave(); } - +/* #ifdef ANDROID #define V8_HOST_ARCH_ARM 1 #define SYS_gettid __NR_gettid #define SYS_tgkill __NR_tgkill #else +*/ #define V8_HOST_ARCH_X64 1 -#endif +/*#endif*/ static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context) { if (!sActiveSampler) return; @@ -184,7 +185,7 @@ } void tgkill(pid_t tgid, pid_t tid, int signalno) { - syscall(SYS_tgkill, tgid, tid, signalno); + /*syscall(SYS_tgkill, tgid, tid, signalno);*/ } //class Sampler::PlatformData : public Malloced { diff -r aa6ad7ab9b43 tools/profiler/sps/platform.h --- a/tools/profiler/sps/platform.h Mon Nov 28 10:34:54 2011 +0000 +++ b/tools/profiler/sps/platform.h Tue Dec 06 08:33:09 2011 -0800 @@ -13,7 +13,7 @@ #include <vector> #define ASSERT(a) MOZ_ASSERT(a) #ifdef ANDROID -#define ENABLE_SPS_LEAF_DATA +/*#define ENABLE_SPS_LEAF_DATA*/ #define LOG(text) __android_log_print(ANDROID_LOG_ERROR, "profiler", "%s", text); #else #define LOG(text) printf("Profiler: %s\n", text) diff -r aa6ad7ab9b43 tools/profiler/sps/sps_sampler.h --- a/tools/profiler/sps/sps_sampler.h Mon Nov 28 10:34:54 2011 +0000 +++ b/tools/profiler/sps/sps_sampler.h Tue Dec 06 08:33:09 2011 -0800 @@ -54,7 +54,8 @@ // we need to make stores are not re-ordered by the compiler // or hardware to make sure the profile is consistent at // every point the signal can fire. -#ifdef ARCH_CPU_ARM_FAMILY +/*#ifdef ARCH_CPU_ARM_FAMILY*/ +#if 1 // TODO Is there something cheaper that will prevent // memory stores from being reordered // Uses: pLinuxKernelMemoryBarrier
Reporter | ||
Updated•13 years ago
|
OS: Linux → Android
Hardware: x86_64 → x86
Comment 1•13 years ago
|
||
Yup, lets clean up this patch and we can try to upstream.
Assignee: nobody → thomas.g.eaton
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•13 years ago
|
||
just to note, we're happy to review and take the patches but its not a priority for the upcoming release
Priority: -- → P5
Reporter | ||
Comment 3•12 years ago
|
||
My fix above is clearly a hack that just enables me to build for x86. I need help from someone who is familiar with the above code to implement a proper solution.
Comment 4•12 years ago
|
||
Assignee: thomas.g.eaton → m_kato
Attachment #596312 -
Flags: review?(bgirard)
Assignee | ||
Comment 5•12 years ago
|
||
The leaf data is an optional feature. I suggest we remove it for this configuration. Does this patch fix the problem?
Updated•12 years ago
|
Attachment #596330 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 6•12 years ago
|
||
I plan on landing this patch when we can confirm that is fixes the issue. Thomas?
Reporter | ||
Comment 7•12 years ago
|
||
Would you like me to test your patch, the patch from Makoto Kato, or both together?
Assignee | ||
Comment 8•12 years ago
|
||
My patch only. It shouldn't compile the code that your patch tries to clean up.
Reporter | ||
Comment 9•12 years ago
|
||
With your patch alone, it builds successfully, but Fennec crashes shortly after running it. I'm trying to determine whether this is a new bug or whether Makoto Kato's patch or my previous hack fix the issue.
Assignee | ||
Comment 10•12 years ago
|
||
Have you attached with GDB to get the crash. The profiler shouldn't be running so I don't see what the crash could be. Perhaps something unrelated. In any case you should be building with only my patch applied.
Reporter | ||
Comment 11•12 years ago
|
||
I didn't attach GDB and haven't ever set it up. The crash I'm seeing appears to be unrelated. I went back to the version of source I pulled on Jan 25th and applied your change while removing mine and everything appeared to work correctly. At that time, these files were located under tools/profiler/sps/.
Assignee | ||
Comment 12•12 years ago
|
||
Excellent, i'll land the patch
Updated•12 years ago
|
Attachment #596312 -
Attachment is obsolete: true
Attachment #596312 -
Flags: review?(bgirard)
Updated•12 years ago
|
Keywords: checkin-needed
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5f655829e1
Keywords: checkin-needed
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fe5f655829e1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Assignee | ||
Updated•11 years ago
|
Component: General → Gecko Profiler
Product: Firefox for Android → Core
Target Milestone: Firefox 13 → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•