Last Comment Bug 681588 - Cleanup wrap malloc and its use on Android
: Cleanup wrap malloc and its use on Android
Status: RESOLVED FIXED
fixed-in-bs
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 680373
Blocks: 696465 680440 696572
  Show dependency treegraph
 
Reported: 2011-08-24 01:37 PDT by Mike Hommey [:glandium]
Modified: 2011-10-22 08:46 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Cleanup wrap malloc and its use on Android (87.11 KB, patch)
2011-08-24 03:57 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Review
Cleanup wrap malloc and its use on Android (21.31 KB, patch)
2011-08-24 05:23 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
Cleanup wrap malloc and its use on Android. NSPR part [Checked in: Comment 8] (66.44 KB, patch)
2011-08-24 05:38 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Review
Cleanup wrap malloc and its use on Android (20.54 KB, patch)
2011-08-29 09:58 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Review
Cleanup wrap malloc and its use on Android [Checked in: Comment 11] (20.16 KB, patch)
2011-08-29 10:21 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Review

Description Mike Hommey [:glandium] 2011-08-24 01:37:10 PDT
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] 2011-08-24 03:57:30 PDT
Created attachment 555354 [details] [diff] [review]
Cleanup wrap malloc and its use on Android

This makes things clearer (IMHO), and fixes Android build without jemalloc.
Comment 2 Mike Hommey [:glandium] 2011-08-24 05:23:10 PDT
Created attachment 555368 [details] [diff] [review]
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] 2011-08-24 05:38:14 PDT
Created attachment 555371 [details] [diff] [review]
Cleanup wrap malloc and its use on Android. NSPR part
[Checked in: Comment 8]
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-08-26 12:25:44 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-08-26 12:30:05 PDT
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] 2011-08-26 13:52:19 PDT
(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] 2011-08-29 09:58:04 PDT
Created attachment 556589 [details] [diff] [review]
Cleanup wrap malloc and its use on Android

Without the typo and the nss file.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2011-08-29 10:08:06 PDT
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] 2011-08-29 10:21:40 PDT
Created attachment 556595 [details] [diff] [review]
Cleanup wrap malloc and its use on Android
[Checked in: Comment 11]

And without the nspr workaround
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-08-31 08:07:42 PDT
http://hg.mozilla.org/mozilla-central/rev/cf4d258fd0f6

Note You need to log in before you can comment on or make changes to this bug.