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.

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.

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

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

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.

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

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.

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

Now with more _MOZBUILD_EXTERNAL_VARIABLES goodness.

I guess doing this in Python is a little nicer.

If you want to use a static list, we might as well separate out that static list so that Windows can use it too.

Likewise, we might as well make Windows build its own sqlite.def file instead of relying on the one built someplace else.

...and I think this one addresses all your previous comments.

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)

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))

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.

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.

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

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

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.

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.

OK, revised approach with review comments addressed.

Here, let's fix things so they actually build properly.

And one small build fix.

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

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

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?

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

OK, so we need this part...

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.

Much simpler thanks to a bunch of logic living in convert_def_file now.

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:

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.

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.

https://hg.mozilla.org/mozilla-central/rev/6afdd52c5b0f https://hg.mozilla.org/mozilla-central/rev/b429ec77a2f6 https://hg.mozilla.org/mozilla-central/rev/769c0d39a5b4 https://hg.mozilla.org/mozilla-central/rev/7eb740dd7f53 https://hg.mozilla.org/mozilla-central/rev/1f1f40053a23