Cleanup wrap malloc and its use on Android

RESOLVED FIXED in mozilla9

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-bs)

Attachments

(2 attachments, 3 obsolete attachments)

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).
Depends on: 680373
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.
Attachment #555354 - Flags: review?(ted.mielczarek)
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
Attachment #555368 - Flags: review?(ted.mielczarek)
Attachment #555354 - Attachment is obsolete: true
Attachment #555354 - Flags: review?(ted.mielczarek)
Created attachment 555371 [details] [diff] [review]
Cleanup wrap malloc and its use on Android. NSPR part
[Checked in: Comment 8]
Attachment #555371 - Flags: review?(ted.mielczarek)
Assignee: nobody → mh+mozilla
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`
?
Attachment #555371 - Flags: review?(ted.mielczarek) → review+
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...
Attachment #555368 - Flags: review?(ted.mielczarek) → review+
(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
Created attachment 556589 [details] [diff] [review]
Cleanup wrap malloc and its use on Android

Without the typo and the nss file.
Attachment #555368 - Attachment is obsolete: true
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
Created attachment 556595 [details] [diff] [review]
Cleanup wrap malloc and its use on Android
[Checked in: Comment 11]

And without the nspr workaround
Attachment #556589 - Attachment is obsolete: true
http://hg.mozilla.org/projects/build-system/rev/14497b5c651e
http://hg.mozilla.org/projects/build-system/rev/cf4d258fd0f6
Whiteboard: fixed-in-bs
http://hg.mozilla.org/mozilla-central/rev/cf4d258fd0f6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Blocks: 696465
Blocks: 696572
Attachment #555371 - Attachment description: Cleanup wrap malloc and its use on Android. NSPR part → Cleanup wrap malloc and its use on Android. NSPR part [Checked in: Comment 8]
Attachment #556595 - Attachment description: Cleanup wrap malloc and its use on Android. → Cleanup wrap malloc and its use on Android [Checked in: Comment 11]
You need to log in before you can comment on or make changes to this bug.