build Gecko's libnss3.so with proper exported symbols on Android, thereby enabling --gc-sections to be effective

RESOLVED FIXED in mozilla33

Status

Firefox Build System
General
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 10 obsolete attachments)

3.23 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.68 KB, patch
glandium
: review+
Details | Diff | Splinter Review
671 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
7.56 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.54 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Right now, we just toss a bunch of stuff into libnss3.so on Android without specifying what's externally visible and what's not.  This strategy hurts us, because on Linux, bug 1011229 has shown that using --gc-sections just on NSS itself is very effective for trimming fat.

We can build nss3.dll on Windows with proper symbol exports, we should be able to do the same on Android.
Current nightly libnss3.so on Android (un-szipped):

[froydnj@cerebro build-android]$ size ~/libnss3.so
   text	   data	    bss	    dec	    hex	filename
1649162	  49060	  14478	1712700	 1a223c	/home/froydnj/libnss3.so

with patch:

[froydnj@cerebro build-android]$ size ~/libnss3.so
   text	   data	    bss	    dec	    hex	filename
1357008	  45616	  14318	1416942	 159eee	/home/froydnj/libnss3.so

which is about ~300k of win, totally worth it.

The szipped files obviously show less of a difference, about ~120K.  Still pretty good.
Created attachment 8431861 [details] [diff] [review]
part 1 - add LD_VERSION_SCRIPT build variable

DEFFILE is Windows-only, we need some equivalent for non-Windows systems.  I
didn't want to overload DEFFILE for Unix-y systems, since they are...sort of
different.  Bikeshedding on the name welcome.
Attachment #8431861 - Flags: review?(mh+mozilla)
Created attachment 8431866 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Android

For the purposes of security/build/, we need to produce a sqlite.def file.
This patch adds the appropriate bits for Android.  I still think this would be
nicer in Python. :p
Attachment #8431866 - Flags: review?(mh+mozilla)
Created attachment 8431868 [details] [diff] [review]
part 3 - use a linker script for libnss3 on Android

Just like it says in the commit message.  A bit ugly with the makefile, but
that's what you get for dealing with NSS .def files...
Attachment #8431868 - Flags: review?(mh+mozilla)
Comment on attachment 8431866 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Android

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

::: db/sqlite3/src/Makefile.in
@@ +26,5 @@
>  export:: sqlite-version.h
>  endif
>  
> +ifeq ($(OS_TARGET),Android)
> +LD_VERSION_SCRIPT = $(CURDIR)/sqlite-processed.def

So, you add a variable for moz.build, and you set it in Makefile.in? ;)

@@ +38,5 @@
> +$(preprocessed_LD_VERSION_SCRIPT): sqlite.def
> +	@$(call py_action,preprocessor,$(DEFINES) $(ACDEFINES) \
> +	  $(srcdir)/sqlite.def -o $@)
> +
> +# Convert to the format we need for the Android linker.

s/the Android linker/ld/

@@ +47,5 @@
> +	echo 'local:' >> $@.tmp
> +	echo '*;' >> $@.tmp
> +	echo '}' >> $@.tmp
> +	mv $@.tmp $@
> +endif

This would probably be better as a python script that does the preprocessing itself.
Attachment #8431866 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8431861 [details] [diff] [review]
part 1 - add LD_VERSION_SCRIPT build variable

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

You should add the variable to _MOZBUILD_EXTERNAL_VARIABLES in config/config.mk
Attachment #8431861 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8431868 [details] [diff] [review]
part 3 - use a linker script for libnss3 on Android

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

::: security/build/Makefile.in
@@ +344,5 @@
>  	grep -v -h -e ^LIBRARY -e ^EXPORTS -e ^\; $^ >> $@.tmp
>  	mv $@.tmp $@
>  endif
>  
> +ifeq (Android,$(OS_TARGET))

No need to limit this to Android. If we ever do MOZ_FOLD_LIBS builds for linux, we'll want to use this.

@@ +347,5 @@
>  
> +ifeq (Android,$(OS_TARGET))
> +nss_static_libs_basenames := $(notdir $(NSS_STATIC_LIBS))
> +nss_static_libs_deffiles := $(join $(NSS_STATIC_DIRS),$(nss_static_libs_basenames:lib%.a=/%.def))
> +NSS_STATIC_LIBS_DEFS := $(wildcard $(addprefix $(srcdir)/../,$(nss_static_libs_deffiles)))

NSS_STATIC_LIBS contains all intermediate libraries that don't normally have .def files. You should derive from NSS_LIBS instead... except the nssutil directory is not nssutil but util... sigh... I wouldn't mind using a static list of files. On the long term, we may not even want to use those files anyways, since we'd like to remove what's not used in nss.

@@ +349,5 @@
> +nss_static_libs_basenames := $(notdir $(NSS_STATIC_LIBS))
> +nss_static_libs_deffiles := $(join $(NSS_STATIC_DIRS),$(nss_static_libs_basenames:lib%.a=/%.def))
> +NSS_STATIC_LIBS_DEFS := $(wildcard $(addprefix $(srcdir)/../,$(nss_static_libs_deffiles)))
> +
> +sqlite_deffile = $(DEPTH)/db/sqlite3/src/sqlite-processed.def

Probably better to read and preprocess that file from here instead (which would make the rules in db/sqlite3/src only useful for !MOZ_FOLD_LIBS builds). Interdependencies between build rules in different directories are tricky, and future changes to the build system are likely to change the order in which things are processed, probably breaking this at the same time.
Attachment #8431868 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #7)
> @@ +347,5 @@
> >  
> > +ifeq (Android,$(OS_TARGET))
> > +nss_static_libs_basenames := $(notdir $(NSS_STATIC_LIBS))
> > +nss_static_libs_deffiles := $(join $(NSS_STATIC_DIRS),$(nss_static_libs_basenames:lib%.a=/%.def))
> > +NSS_STATIC_LIBS_DEFS := $(wildcard $(addprefix $(srcdir)/../,$(nss_static_libs_deffiles)))
> 
> NSS_STATIC_LIBS contains all intermediate libraries that don't normally have
> .def files. You should derive from NSS_LIBS instead... except the nssutil
> directory is not nssutil but util... sigh... I wouldn't mind using a static
> list of files. On the long term, we may not even want to use those files
> anyways, since we'd like to remove what's not used in nss.

I guess this makes sense.  I 'll fix.

> @@ +349,5 @@
> > +nss_static_libs_basenames := $(notdir $(NSS_STATIC_LIBS))
> > +nss_static_libs_deffiles := $(join $(NSS_STATIC_DIRS),$(nss_static_libs_basenames:lib%.a=/%.def))
> > +NSS_STATIC_LIBS_DEFS := $(wildcard $(addprefix $(srcdir)/../,$(nss_static_libs_deffiles)))
> > +
> > +sqlite_deffile = $(DEPTH)/db/sqlite3/src/sqlite-processed.def
> 
> Probably better to read and preprocess that file from here instead (which
> would make the rules in db/sqlite3/src only useful for !MOZ_FOLD_LIBS
> builds).

I dunno, duplicating the rules from db/sqlite3/src/ doesn't seem valuable for this case.  I guess I'll fix this up for the Windows case, too.
Created attachment 8433524 [details] [diff] [review]
part 1 - add LD_VERSION_SCRIPT build variable

Now with more _MOZBUILD_EXTERNAL_VARIABLES goodness.
Attachment #8431861 - Attachment is obsolete: true
Attachment #8433524 - Flags: review?(mh+mozilla)
Created attachment 8433525 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Android

I guess doing this in Python is a little nicer.
Attachment #8431866 - Attachment is obsolete: true
Attachment #8433525 - Flags: review?(mh+mozilla)
Created attachment 8433528 [details] [diff] [review]
part 3 - use a static list of NSS def files for MOZ_FOLD_LIBS groveling

If you want to use a static list, we might as well separate out that static
list so that Windows can use it too.
Attachment #8433528 - Flags: review?(mh+mozilla)
Created attachment 8433530 [details] [diff] [review]
part 4a - don't depend on db/sqlite3/src/sqlite-processed.def from security/build/

Likewise, we might as well make Windows build its own sqlite.def file instead
of relying on the one built someplace else.
Attachment #8433530 - Flags: review?(mh+mozilla)
Created attachment 8433531 [details] [diff] [review]
part 4 - use a linker script for libnss3 on Android

...and I think this one addresses all your previous comments.
Attachment #8431868 - Attachment is obsolete: true
Attachment #8433531 - Flags: review?(mh+mozilla)
Attachment #8433524 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8433525 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Android

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

::: db/sqlite3/src/Makefile.in
@@ +39,5 @@
> +	  $(srcdir)/sqlite.def -o $@)
> +
> +# Convert to the format we need for ld.
> +$(LD_VERSION_SCRIPT): $(preprocessed_LD_VERSION_SCRIPT)
> +	@$(call py_action,convert_def_file,$^ $@)

Now that you're doing this in python, you could move the preprocessing there ;)

::: python/mozbuild/mozbuild/action/convert_def_file.py
@@ +19,5 @@
> +
> +def main(args):
> +    with open(args[0], 'r') as input:
> +        with open(args[1], 'w') as output:
> +            symbols = dropuntil(lambda line: 'EXPORTS' in line, input)

Note it's possible to have a symbol after EXPORTS, so just to avoid any surprise if that happens, I'd rather error out in that case than dropping said symbol unknowingly. Also, the syntax for def files allows keywords and assigning aliases, it would be better to error out in that case too. (could be a simple sanity check like len(line.split()) == 1 and '=' not in line)
Attachment #8433525 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8433528 [details] [diff] [review]
part 3 - use a static list of NSS def files for MOZ_FOLD_LIBS groveling

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

::: security/build/Makefile.in
@@ +338,5 @@
> +  nss/lib/nss/nss.def \
> +  nss/lib/ssl/ssl.def \
> +  nss/lib/smime/smime.def \
> +  $(NULL)
> +nss_def_files := $(wildcard $(addprefix $(srcdir)/../,$(nss_relative_def_files)))

no $(wildcard) here. Could also make that just one definition:
nss_def_files := $(addprefix $(srcdir)/../, \
  nss/lib/util/nssutil.def \
  nss/lib/nss/nss.def \
  nss/lib/ssl/ssl.def \
  nss/lib/smime/smime.def \
)

@@ +341,5 @@
> +  $(NULL)
> +nss_def_files := $(wildcard $(addprefix $(srcdir)/../,$(nss_relative_def_files)))
> +
> +# Try to guard against NSS renaming things out from underneath us.
> +ifneq ($(words $(nss_relative_def_files)),$(words $(nss_def_files)))

ifneq ($(nss_def_files),$(wildcard $(nss_def_files))
Attachment #8433528 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8433531 [details] [diff] [review]
part 4 - use a linker script for libnss3 on Android

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

::: security/build/Makefile.in
@@ +379,5 @@
> +	# NSPR exports symbols with symbol visibility rather than with
> +	# deffiles like NSS does.  So we need to explicitly specify that
> +	# NSPR's symbols should be globally visible.  Otherwise, they'd
> +	# match the |local: *| rule below.
> +	echo '  PR_*; _PR_*; PL_*;' >> $@.tmp

Create a dummy nspr.def with EXPORTS for PR_*, _PR_* and PL_*, make convert_def_file.py take many def files, and use that.
Attachment #8433531 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8433530 [details] [diff] [review]
part 4a - don't depend on db/sqlite3/src/sqlite-processed.def from security/build/

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

Depending what you do for part 2 and 4, it might not matter. Resetting flag for now.
Attachment #8433530 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #14)
> ::: db/sqlite3/src/Makefile.in
> @@ +39,5 @@
> > +	  $(srcdir)/sqlite.def -o $@)
> > +
> > +# Convert to the format we need for ld.
> > +$(LD_VERSION_SCRIPT): $(preprocessed_LD_VERSION_SCRIPT)
> > +	@$(call py_action,convert_def_file,$^ $@)
> 
> Now that you're doing this in python, you could move the preprocessing there
> ;)

I agree that would be better, and I looked at that, but preprocessor.py looked extremely unfriendly to doing anything other than file-to-file preprocessing.  Maybe I just wasn't looking in the right place.

> ::: python/mozbuild/mozbuild/action/convert_def_file.py
> @@ +19,5 @@
> > +
> > +def main(args):
> > +    with open(args[0], 'r') as input:
> > +        with open(args[1], 'w') as output:
> > +            symbols = dropuntil(lambda line: 'EXPORTS' in line, input)
> 
> Note it's possible to have a symbol after EXPORTS, so just to avoid any
> surprise if that happens, I'd rather error out in that case than dropping
> said symbol unknowingly. Also, the syntax for def files allows keywords and
> assigning aliases, it would be better to error out in that case too. (could
> be a simple sanity check like len(line.split()) == 1 and '=' not in line)

Ugh.  OK, will fix that.

(In reply to Mike Hommey [:glandium] from comment #16)
> ::: security/build/Makefile.in
> @@ +379,5 @@
> > +	# NSPR exports symbols with symbol visibility rather than with
> > +	# deffiles like NSS does.  So we need to explicitly specify that
> > +	# NSPR's symbols should be globally visible.  Otherwise, they'd
> > +	# match the |local: *| rule below.
> > +	echo '  PR_*; _PR_*; PL_*;' >> $@.tmp
> 
> Create a dummy nspr.def with EXPORTS for PR_*, _PR_* and PL_*, make
> convert_def_file.py take many def files, and use that.

I don't think this is the right thing to do; the .def file format makes no allowances for wildcarded symbols.  Trying to stuff these symbols into a .def file just seems like needless complexity.

WDYT?
Flags: needinfo?(mh+mozilla)
(In reply to Nathan Froyd (:froydnj) from comment #18)
> > Now that you're doing this in python, you could move the preprocessing there
> > ;)
> 
> I agree that would be better, and I looked at that, but preprocessor.py
> looked extremely unfriendly to doing anything other than file-to-file
> preprocessing.  Maybe I just wasn't looking in the right place.

That's true, but it's easy to use StringIO.

> > Create a dummy nspr.def with EXPORTS for PR_*, _PR_* and PL_*, make
> > convert_def_file.py take many def files, and use that.
> 
> I don't think this is the right thing to do; the .def file format makes no
> allowances for wildcarded symbols.  Trying to stuff these symbols into a
> .def file just seems like needless complexity.
> 
> WDYT?

Between a shell script in a Makefile, and a small abuse of .def allowing to use a python script, I'd take the latter any day.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8433530 [details] [diff] [review]
part 4a - don't depend on db/sqlite3/src/sqlite-processed.def from security/build/

I'm not sure which combination of {discard this, keep this} x {method a, method b} you were thinking in comment 17.  I don't think it matters, though: I argue that if we're not going to depend on sqlite-processed.def for the Linux case, to be consistent, we shouldn't depend on it in the Windows case either.  Hence this patch and hence the re-r? request.
Attachment #8433530 - Flags: review?(mh+mozilla)
Created attachment 8435028 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Android

I know you r+'d this already, but I figure with a rewrite to do preprocessing
in the convert_def_file.py script and making db/sqlite3/src/ do things for
Linux, too, it's probably worth another look.
Attachment #8433525 - Attachment is obsolete: true
Attachment #8435028 - Flags: review?(mh+mozilla)
Created attachment 8435034 [details] [diff] [review]
part 4 - use a linker script for libnss3 on Linux-like OSes

OK, revised approach with review comments addressed.
Attachment #8433531 - Attachment is obsolete: true
Attachment #8435034 - Flags: review?(mh+mozilla)
Created attachment 8435056 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Linux-like OSes

Here, let's fix things so they actually build properly.
Attachment #8435028 - Attachment is obsolete: true
Attachment #8435028 - Flags: review?(mh+mozilla)
Attachment #8435056 - Flags: review?(mh+mozilla)
Created attachment 8435058 [details] [diff] [review]
part 4 - use a linker script for libnss3 on Linux-like OSes

And one small build fix.
Attachment #8435034 - Attachment is obsolete: true
Attachment #8435034 - Flags: review?(mh+mozilla)
Attachment #8435058 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #7)
> > +sqlite_deffile = $(DEPTH)/db/sqlite3/src/sqlite-processed.def
> 
> Probably better to read and preprocess that file from here instead (which
> would make the rules in db/sqlite3/src only useful for !MOZ_FOLD_LIBS
> builds). Interdependencies between build rules in different directories are
> tricky, and future changes to the build system are likely to change the
> order in which things are processed, probably breaking this at the same time.

One problem with doing this, as I discovered on try, is that the "real" sqlite.def file gets processed with -DSQLITE_DEBUG by virtue of the moz.build file in the same directory.  If you do the processing here, there's no -DSQLITE_DEBUG, and you get linking failures, because the necessary symbols aren't exported from the library.  Possible solutions:

- add -DSQLITE_DEBUG appropriately here;
- change the SQLITE_DEBUG in the .def file to DEBUG;
- just use the already-processed copy, instead of redoing the preprocessing.
Comment on attachment 8435056 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Linux-like OSes

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

::: db/sqlite3/src/Makefile.in
@@ +27,5 @@
>  
>  export:: sqlite-version.h
>  endif
>  
> +ifeq ($(OS_ARCH),Linux)

You want a combination of ! MOZ_FOLD_LIBS and GCC_USE_GNU_LD (which is not AC_SUBSTed currently, so you'd need to add that)

::: db/sqlite3/src/moz.build
@@ +73,5 @@
>  if CONFIG['OS_ARCH'] == 'WINNT':
>      RCFILE  = 'sqlite.rc'
>      RESFILE = 'sqlite.res'
>  
> +if CONFIG['OS_ARCH'] == 'Linux':

Likewise

::: python/mozbuild/mozbuild/action/convert_def_file.py
@@ +6,5 @@
> +# script, applying any necessary preprocessing.
> +
> +import itertools
> +import re
> +import StringIO

from StringIO import StringIO
or better (if that works, because it can have different behaviour iirc):
from io import StringIO

@@ +12,5 @@
> +
> +from mozbuild.preprocessor import Preprocessor
> +
> +def preprocess_file(pp, deffile):
> +    output = StringIO.StringIO()

StringIO()

@@ +15,5 @@
> +def preprocess_file(pp, deffile):
> +    output = StringIO.StringIO()
> +    with open(deffile, 'r') as input:
> +        pp.processFile(input=input, output=output)
> +    return output.getvalue().split('\n')

.splitlines()

@@ +23,5 @@
> +def extract_symbols(pp, deffile):
> +    lines = preprocess_file(pp, deffile)
> +
> +    # Filter comments.
> +    nocomments = itertools.imap(lambda s: COMMENT.sub('', s), lines)

nocomments = iter(COMMENT.sub('', s) for s in lines)

could probably add a strip() in there too.

@@ +24,5 @@
> +    lines = preprocess_file(pp, deffile)
> +
> +    # Filter comments.
> +    nocomments = itertools.imap(lambda s: COMMENT.sub('', s), lines)
> +    lines = itertools.ifilter(lambda s: len(s) != 0, nocomments)

lines = iter(s for s in nocomments if len(s))

@@ +38,5 @@
> +        else:
> +            fields = line.split()
> +
> +        # We don't support aliases, ordinals, or anything like that.
> +        if len(fields) != 1 or '=' in fields[0]:

Does the syntax of deffiles require that the equal sign is not preceded by whitespace?
Attachment #8435056 - Flags: review?(mh+mozilla) → review+
(In reply to Nathan Froyd (:froydnj) from comment #25)
> - change the SQLITE_DEBUG in the .def file to DEBUG;

Looks to me like we could go with this.
Comment on attachment 8433530 [details] [diff] [review]
part 4a - don't depend on db/sqlite3/src/sqlite-processed.def from security/build/

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

I think you can leave that alone for now, because windows uses a different code path. Or if you do want to care about it, then you should also avoid creating the preprocessed file in db/sqlite3/src/ as well.
Attachment #8433530 - Flags: review?(mh+mozilla)
Comment on attachment 8435058 [details] [diff] [review]
part 4 - use a linker script for libnss3 on Linux-like OSes

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

::: security/build/Makefile.in
@@ +369,5 @@
> +
> +nss3.def: $(nss_def_files) $(sqlite_nspr_def_file)
> +	echo '{' > $@.tmp
> +	echo 'global:' >> $@.tmp
> +	grep -v -h -e ^LIBRARY -e ^EXPORTS -e ^\;+ -e \;- $(nss_def_files) | sed  -e 's/ DATA //' -e 's/;;//' >> $@.tmp

Why are you not using convert_def_file for the nss def files?
Attachment #8435058 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #29)
> ::: security/build/Makefile.in
> @@ +369,5 @@
> > +
> > +nss3.def: $(nss_def_files) $(sqlite_nspr_def_file)
> > +	echo '{' > $@.tmp
> > +	echo 'global:' >> $@.tmp
> > +	grep -v -h -e ^LIBRARY -e ^EXPORTS -e ^\;+ -e \;- $(nss_def_files) | sed  -e 's/ DATA //' -e 's/;;//' >> $@.tmp
> 
> Why are you not using convert_def_file for the nss def files?

Because of this giant comment at the top of all nss def files:

;+# OK, this file is meant to support SUN, LINUX, AIX and WINDOWS
;+#   1. For all unix platforms, the string ";-"  means "remove this line"
;+#   2. For all unix platforms, the string " DATA " will be removed from any 
;+#	line on which it occurs.
;+#   3. Lines containing ";+" will have ";+" removed on SUN and LINUX.
;+#      On AIX, lines containing ";+" will be removed.  
;+#   4. For all unix platforms, the string ";;" will thave the ";;" removed.
;+#   5. For all unix platforms, after the above processing has taken place,
;+#    all characters after the first ";" on the line will be removed.  
;+#    And for AIX, the first ";" will also be removed.
;+#  This file is passed directly to windows. Since ';' is a comment, all UNIX
;+#   directives are hidden behind ";", ";+", and ";-"

I guess one could munge these rules into convert_def_file, but it seems wrong to stick rules for NSS files directly into convert_def_file.
Created attachment 8438634 [details] [diff] [review]
part 2a - export GCC_USE_GNU_LD from configure

OK, so we need this part...
Attachment #8438634 - Flags: review?(mh+mozilla)
Created attachment 8438635 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Linux-like OSes

I think this addresses all the Pythonic review comments.  It also makes
convert_def_file.py understand NSS .def files, so part 4 is pretty
straightforward.
Attachment #8435056 - Attachment is obsolete: true
Attachment #8438635 - Flags: review?(mh+mozilla)
Created attachment 8438636 [details] [diff] [review]
part 4 - use a linker script for libnss3 on Linux-like OSes

Much simpler thanks to a bunch of logic living in convert_def_file now.
Attachment #8433530 - Attachment is obsolete: true
Attachment #8435058 - Attachment is obsolete: true
Attachment #8438636 - Flags: review?(mh+mozilla)
Attachment #8438634 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8438635 [details] [diff] [review]
part 2 - make db/sqlite3/src/ produce a version script for Linux-like OSes

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

::: db/sqlite3/src/Makefile.in
@@ +27,5 @@
>  
>  export:: sqlite-version.h
>  endif
>  
> +ifeq ($(OS_ARCH),Linux)

I'd rather not check for OS_ARCH at all. But maybe that'll break mingw, so if we need an OS_ARCH test, it should be it not being WINNT.

::: python/mozbuild/mozbuild/action/convert_def_file.py
@@ +7,5 @@
> +
> +import itertools
> +import re
> +import sys
> +from StringIO import StringIO

Did from io import StringIO fail?

@@ +46,5 @@
> +    with open(deffile, 'r') as input:
> +        for line in input:
> +            # Rule 2, and then rule 4.
> +            yield line.replace(' DATA ', '').replace(';;', '')
> +        

trailing whitespaces.

@@ +85,5 @@
> +    options, deffiles = optparser.parse_args(args)
> +
> +    symbols = set()
> +    for f in options.nss_files:
> +        symbols |= extract_symbols(nss_preprocess_file(f))

I'm tempted to say just apply the nss filter on every file and don't care about the --nss-file flag at all. Meh.

@@ +98,5 @@
> +local:
> +  *;
> +};
> +"""
> +    with open(options.output, 'w') as f:

from mozbuild.util import FileAvoidWrite
with FileAvoidWrite(options.output) as f:
Attachment #8438635 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8438636 [details] [diff] [review]
part 4 - use a linker script for libnss3 on Linux-like OSes

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

More straightforward.

::: security/build/Makefile.in
@@ +354,5 @@
>  	grep -v -h -e ^LIBRARY -e ^EXPORTS -e ^\; $^ >> $@.tmp
>  	mv $@.tmp $@
>  endif
>  
> +ifeq (Linux,$(OS_ARCH))

Same as for the other patch.
Attachment #8438636 - Flags: review?(mh+mozilla) → review+
Depends on: 1025139
Good work on this!

Is there another bug for optimizing the minimizig of the *.def files? Soon we're going to need to create our own *.def files that are a subset of the contents of the NSS *.def files, in order to get the biggest size wins.
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #36)
> Good work on this!

Thanks!

> Is there another bug for optimizing the minimizig of the *.def files? Soon
> we're going to need to create our own *.def files that are a subset of the
> contents of the NSS *.def files, in order to get the biggest size wins.

There's not; if you want to file one, please feel free.  Otherwise, I'll file one early next week.
(In reply to Nathan Froyd (:froydnj) from comment #37)
> > Is there another bug for optimizing the minimizig of the *.def files? Soon
> > we're going to need to create our own *.def files that are a subset of the
> > contents of the NSS *.def files, in order to get the biggest size wins.
> 
> There's not; if you want to file one, please feel free.  Otherwise, I'll
> file one early next week.

This is now bug 1025998.

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.