Closed Bug 957450 Opened 6 years ago Closed 6 years ago

jemalloc hit compile error in gonk-kk build

Categories

(Firefox OS Graveyard :: GonkIntegration, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: seinlin, Assigned: seinlin)

References

Details

Attachments

(1 file, 2 obsolete files)

In gonk-kk build, jemalloc hit a compile error.
Blocks: gonk-kk
Attached patch bug-957450-fix.patch (obsolete) — Splinter Review
Hi Iacob, Can you help me have a look on this patch?
Assignee: nobody → kli
Attachment #8357839 - Flags: review?(iacobcatalin)
Attached patch bug-957450-fix-2.patch (obsolete) — Splinter Review
Correct the build error on ICS and JB. 

Try result: https://tbpl.mozilla.org/?tree=Try&rev=52f5fffaf887
Attachment #8357839 - Attachment is obsolete: true
Attachment #8357839 - Flags: review?(iacobcatalin)
Attachment #8358263 - Flags: review?(iacobcatalin)
Comment on attachment 8358263 [details] [diff] [review]
bug-957450-fix-2.patch

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

r=me for this simple build fix. I will leave a feedback up for iacobcatalin.

::: memory/build/malloc_decls.h
@@ +14,5 @@
>  #  define malloc_decls_h
>  
>  #  include "jemalloc_types.h"
>  
> +#  if defined(__linux__) && (ANDROID_VERSION < 19)

Please change this to

if defined(__linux__) || (defined(MOZ_MEMORY_ANDROID) && ANDROID_VERSION < 19)

thats cleaner
Attachment #8358263 - Flags: review?(iacobcatalin)
Attachment #8358263 - Flags: review+
Attachment #8358263 - Flags: feedback?(iacobcatalin)
Comment on attachment 8358263 [details] [diff] [review]
bug-957450-fix-2.patch

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

Sorry for the late response.

With Andreas' suggested change, +1 from me. Just noticed the mercurial commit in the patch lists me as reviewer, you should change it for the final commit.

::: memory/build/malloc_decls.h
@@ +14,5 @@
>  #  define malloc_decls_h
>  
>  #  include "jemalloc_types.h"
>  
> +#  if defined(__linux__) && (ANDROID_VERSION < 19)

Definitely agreed to Andreas' comment. I was quite confused initially since I had no idea what this would evaluate to on non Android Linux. I had to look up and read that macros left undefined it will evaluate to 0 so the test will pass but it caused some head scratching.
Attachment #8358263 - Flags: feedback?(iacobcatalin) → feedback+
I try the following two on try server. It will break other build. So I think the current r+ patch already the correct one. And I think for non Android Linux "AND a condition which will be evaluated as true " will not be a problem. 

if defined(__linux__) && (defined(MOZ_MEMORY_ANDROID) && ANDROID_VERSION < 19)
https://tbpl.mozilla.org/?tree=Try&rev=38770d3c20a6

if defined(__linux__) || (defined(MOZ_MEMORY_ANDROID) && ANDROID_VERSION < 19)
https://tbpl.mozilla.org/?tree=Try&rev=aacd12dbcc5c
Ok can you please back out my patch? Thanks.
https://hg.mozilla.org/mozilla-central/rev/977d6be3df40
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think this will be more clear to know that why we need to check ANDROID_VERSION.
Attachment #8358263 - Attachment is obsolete: true
Attachment #8360166 - Flags: review?(gal)
Comment on attachment 8360166 [details] [diff] [review]
Use marco define explicitly

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

::: memory/build/malloc_decls.h
@@ +14,5 @@
>  #  define malloc_decls_h
>  
>  #  include "jemalloc_types.h"
>  
> +#  if defined(__linux__) && (!defined(MOZ_MEMORY_ANDROID) || ANDROID_VERSION < 19)

this should be linux || ...
Attachment #8360166 - Flags: review?(gal) → review+
(In reply to Andreas Gal :gal from comment #11)
> Comment on attachment 8360166 [details] [diff] [review]
> Use marco define explicitly
> 
> Review of attachment 8360166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: memory/build/malloc_decls.h
> @@ +14,5 @@
> >  #  define malloc_decls_h
> >  
> >  #  include "jemalloc_types.h"
> >  
> > +#  if defined(__linux__) && (!defined(MOZ_MEMORY_ANDROID) || ANDROID_VERSION < 19)
> 
> this should be linux || ...

Hmm, I think Android/Gonk is actually a subset of Linux so that wouldn't work.
I see. Then its actually correct. Alright never mind lets land it :)
https://hg.mozilla.org/mozilla-central/rev/ee9a510f6c8f
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 1083116
You need to log in before you can comment on or make changes to this bug.