681588 - Cleanup wrap malloc and its use on Android

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

There are various issues with how wrap malloc is being used, from the nonsense of the variable names to the lack of flags when building C programs. It also happens that android builds without jemalloc fail (and even if they wouldn't fail at build time, they'd fail at run time on devices without enough free space on the internal flash).

Comment 1

[Mike Hommey [:glandium]]

Attachment: Cleanup wrap malloc and its use on Android

This makes things clearer (IMHO), and fixes Android build without jemalloc.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Cleanup wrap malloc and its use on Android

Without the nspr part, and with a temporary workaround until the nspr part lands

Comment 3

[Mike Hommey [:glandium]]

Attachment: Cleanup wrap malloc and its use on Android. NSPR part [Checked in: Comment 8]

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 555371 [details] [diff] [review]
Cleanup wrap malloc and its use on Android. NSPR part
[Checked in: Comment 8]

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

::: nsprpub/configure.in
@@ +3180,5 @@
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=malloc,--wrap=calloc,--wrap=valloc,--wrap=free,--wrap=realloc,--wrap=memalign"
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=__builtin_new,--wrap=__builtin_vec_new,--wrap=__builtin_delete,--wrap=__builtin_vec_delete"
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=PR_Free,--wrap=PR_Malloc,--wrap=PR_Calloc,--wrap=PR_Realloc"
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=strdup,--wrap=strndup"
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=posix_memalign,--wrap=malloc_usable_size"

Can you maybe do these as a heredoc or something?
WRAP_LDFLAGS=`cat<<EOF
${WRAP_LDFLAGS} ...
...
...
EOF`
?

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 555368 [details] [diff] [review]
Cleanup wrap malloc and its use on Android

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

::: build/unix/elfhack/Makefile.in
@@ +74,5 @@
>    $(NULL)
>  
>  libs:: $(CSRCS:.c=.$(OBJ_SUFFIX))
>  
> +WRAP_FLAGS=

You probably meant WRAP_LDFLAGS= here, right?

::: configure.in
@@ +7473,5 @@
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=malloc,--wrap=calloc,--wrap=valloc,--wrap=free,--wrap=realloc,--wrap=memalign"
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=__builtin_new,--wrap=__builtin_vec_new,--wrap=__builtin_delete,--wrap=__builtin_vec_delete"
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=PR_Free,--wrap=PR_Malloc,--wrap=PR_Calloc,--wrap=PR_Realloc"
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=strdup,--wrap=strndup"
> +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=posix_memalign,--wrap=malloc_usable_size"

Same comment here as in the NSPR patch.

::: security/coreconf/OS2.mk
@@ +70,5 @@
>  HIGHMEM_LDFLAG          = -Zhigh-mem
>  endif
>  
>  ifndef NO_SHARED_LIB
> +WRAP_LDFLAGS            = 

This is an NSS file...

Comment 6

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #5)
> > +WRAP_FLAGS=
> 
> You probably meant WRAP_LDFLAGS= here, right?

Yes
 
> ::: security/coreconf/OS2.mk
> > +WRAP_LDFLAGS            = 
> 
> This is an NSS file...

Oops

(In reply to Ted Mielczarek [:ted, :luser] from comment #4)
> Comment on attachment 555371 [details] [diff] [review]
> Cleanup wrap malloc and its use on Android. NSPR part
> 
> Review of attachment 555371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: nsprpub/configure.in
> @@ +3180,5 @@
> > +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=malloc,--wrap=calloc,--wrap=valloc,--wrap=free,--wrap=realloc,--wrap=memalign"
> > +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=__builtin_new,--wrap=__builtin_vec_new,--wrap=__builtin_delete,--wrap=__builtin_vec_delete"
> > +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=PR_Free,--wrap=PR_Malloc,--wrap=PR_Calloc,--wrap=PR_Realloc"
> > +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=strdup,--wrap=strndup"
> > +        WRAP_LDFLAGS="${WRAP_LDFLAGS} -Wl,--wrap=posix_memalign,--wrap=malloc_usable_size"
> 
> Can you maybe do these as a heredoc or something?
> WRAP_LDFLAGS=`cat<<EOF
> ${WRAP_LDFLAGS} ...
> ...
> ...
> EOF`
> ?

Putting it on several lines doesn't work because the variable then contains newlines, and that messes up the sed used to generate autoconf.mk.
  WRAP_LDFLAGS=`cat <<EOF | xargs echo
  ${WRAP_LDFLAGS}
  ...
  ...
EOF`

would work, but it's somehow ugly, isn't it? Plus the EOF needs not to be indented. Except if we do
    WRAP_LDFLAGS=`cat <<"    EOF" | xargs echo
    ...
    EOF

Comment 7

[Mike Hommey [:glandium]]

Attachment: Cleanup wrap malloc and its use on Android

Without the typo and the nss file.

Comment 8

[(not currently active) Ted Mielczarek]

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.313; previous revision: 1.312
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.315; previous revision: 1.314
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/nsprpub/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 1.45; previous revision: 1.44
done
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.84; previous revision: 3.83
done
Checking in lib/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.25; previous revision: 1.24
done
Checking in pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.68; previous revision: 1.67
done

Comment 9

[Mike Hommey [:glandium]]

Attachment: Cleanup wrap malloc and its use on Android [Checked in: Comment 11]

And without the nspr workaround

Comment 10

[Mike Hommey [:glandium]]

http://hg.mozilla.org/projects/build-system/rev/14497b5c651e
http://hg.mozilla.org/projects/build-system/rev/cf4d258fd0f6

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/cf4d258fd0f6