957450 - jemalloc hit compile error in gonk-kk build

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Kai-Zhen Li [:seinlin][:kli]]

In gonk-kk build, jemalloc hit a compile error.

Comment 1

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-957450-fix.patch

Hi Iacob, Can you help me have a look on this patch?

Comment 2

[Kai-Zhen Li [:seinlin][:kli]]

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=16f3a5ab677a

Comment 3

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: bug-957450-fix-2.patch

Correct the build error on ICS and JB. 

Try result: https://tbpl.mozilla.org/?tree=Try&rev=52f5fffaf887

Comment 4

[Andreas Gal :gal]

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

Comment 5

[Catalin Iacob]

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.

Comment 6

[Andreas Gal :gal]

https://hg.mozilla.org/integration/mozilla-inbound/rev/977d6be3df40

Comment 7

[Kai-Zhen Li [:seinlin][:kli]]

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

Comment 8

[Andreas Gal :gal]

Ok can you please back out my patch? Thanks.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/977d6be3df40

Comment 10

[Kai-Zhen Li [:seinlin][:kli]]

Attachment: Use marco define explicitly

I think this will be more clear to know that why we need to check ANDROID_VERSION.

Comment 11

[Andreas Gal :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 || ...

Comment 12

[Marco Chen [:mchen]]

backout patch committed in comment 9.

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

Comment 13

[Michael Wu [:mwu]]

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

Comment 14

[Andreas Gal :gal]

I see. Then its actually correct. Alright never mind lets land it :)

Comment 15

[Marco Chen [:mchen]]

https://tbpl.mozilla.org/?tree=Try&rev=dd5a3c32d9b8. All Green.

Comment 16

[Marco Chen [:mchen]]

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

Comment 17

[Kai-Zhen Li [:seinlin][:kli]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=dd5a3c32d9b8

Comment 18

[Ed Morley [:emorley]]

(In reply to Marco Chen [:mchen] from comment #12)
> backout patch committed in comment 9.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/aa086509b9db

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/aa086509b9db

Comment 19

[Ed Morley [:emorley]]

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