708733 - Profiler tool has ARM specific code that breaks x86 Android

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P5)

Description

[Thomas G Eaton]

Attachment: moz_x86_android

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

Comment 1

[Doug Turner (:dougt)]

Yup, lets clean up this patch and we can try to upstream.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

just to note, we're happy to review and take the patches but its not a priority for the upcoming release

Comment 3

[Thomas G Eaton]

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

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Disable leaf data for non ARM Android

The leaf data is an optional feature. I suggest we remove it for this configuration.

Does this patch fix the problem?

Comment 6

[Benoit Girard (:BenWa)]

I plan on landing this patch when we can confirm that is fixes the issue. Thomas?

Comment 7

[Thomas G Eaton]

Would you like me to test your patch, the patch from Makoto Kato, or both together?

Comment 8

[Benoit Girard (:BenWa)]

My patch only. It shouldn't compile the code that your patch tries to clean up.

Comment 9

[Thomas G Eaton]

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.

Comment 10

[Benoit Girard (:BenWa)]

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.

Comment 11

[Thomas G Eaton]

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/.

Comment 12

[Benoit Girard (:BenWa)]

Excellent, i'll land the patch

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5f655829e1

Comment 14

[Phil Ringnalda (:philor)]

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