537829 - Allow NSS to build for Android

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Michael Wu [:mwu]]

On Android, the system headers do not compile with -ansi, so NSS does not compile. The placement of -ansi in the line setting OS_CFLAGS in Linux.mk prevents -ansi from being overridden without forcing a completely custom OS_CFLAGS.

Comment 1

[Michael Wu [:mwu]]

Attachment: Allow -ansi to be overridden by other CFLAGS

This moves -ansi to the beginning of the line so -ansi can be overridden by DSO_CFLAGS or any other variable after.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 419984 [details] [diff] [review]
Allow -ansi to be overridden by other CFLAGS

I'm not an NSS peer. You probably want wtc.

Comment 3

[Wan-Teh Chang]

Comment on attachment 419984 [details] [diff] [review]
Allow -ansi to be overridden by other CFLAGS

This change is too subtle.  It definitely needs a comment to explain
why you need -ansi to be before DSO_CFLAGS.

But I think we should take this opportunity to remove -ansi, which will
also make -D_POSIX_SOURCE -D_BSD_SOURCE unnecessary.  Ted, what do you think?

I believe -ansi was added because of a misunderstanding -- we thought -ansi
means "Use ANSI C instead of Traditional C", but GCC's -ansi flag actually
has a different meaning -- it also restricts the library functions declared
by system headers to those in the ANSI Standard C Library.  This is why we
had to add -D_POSIX_SOURCE -D_BSD_SOURCE so we can use more system library
functions.

Mozilla's configure.in doesn't use -ansi.

Comment 4

[(not currently active) Ted Mielczarek]

The GCC manual claims that -ansi is equivalent to -std=c89 when compiling C:
http://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html#C-Dialect-Options

It also defines __STRICT_ANSI__, which is probably what is causing the pain with the system headers, I would assume. (-std=c89 does the same, from my testing.)

Comment 5

[Michael Wu [:mwu]]

Attachment: Remove -ansi from OS_CFLAGS

Well, eliminating -ansi sounds even better to me. This patch also removes -D_POSIX_SOURCE and -D_BSD_SOURCE. What do you think?

Comment 6

[Wan-Teh Chang]

Comment on attachment 420785 [details] [diff] [review]
Remove -ansi from OS_CFLAGS

Michael, thanks for the patch.  Did you test this patch?
Yesterday I tested the same change, and I found that I
had to remove -D_XOPEN_SOURCE also.  Unfortunately we
add -D_XOPEN_SOURCE to OS_REL_CFLAGS in multiple places
in Linux.mk, so the patch became much larger (but
straightforward).

I just did a search for LINUX1_2 in our source tree:
http://mxr.mozilla.org/security/search?string=LINUX1_2

None of our code is using LINUX1_2, so I suggest that
we also remove -DLINUX1_2 from OS_REL_CFLAGS in Linux.mk.

Comment 7

[Michael Wu [:mwu]]

(In reply to comment #6)
> (From update of attachment 420785 [details] [diff] [review])
> Michael, thanks for the patch.  Did you test this patch?
> Yesterday I tested the same change, and I found that I
> had to remove -D_XOPEN_SOURCE also.  Unfortunately we
> add -D_XOPEN_SOURCE to OS_REL_CFLAGS in multiple places
> in Linux.mk, so the patch became much larger (but
> straightforward).
> 
Yes, I tested it but it may not have been enough since I was cross compiling for android. I'll rerequest review after making sure things are okay when compiling against glibc.

Comment 8

[Wan-Teh Chang]

Ah, I see.  Android's system headers may not be checking _XOPEN_SOURCE.

Perhaps I should just ask you to test my patch on Android?

Comment 9

[Michael Wu [:mwu]]

(In reply to comment #8)
> Ah, I see.  Android's system headers may not be checking _XOPEN_SOURCE.
> 
> Perhaps I should just ask you to test my patch on Android?

Sure, sounds good.

Comment 10

[Wan-Teh Chang]

Attachment: Remove -ansi -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE -DLINUX1_2

-D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE are necessary because -ansi
restricts the Linux system headers to declare only the ANSI Standard C
Library functions.  So if we remove -ansi, we can also remove them.

This MXR query shows that LINUX1_2 is not being used:
http://mxr.mozilla.org/security/search?string=LINUX1_2

So we can also remove -DLINUX1_2.

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 420827 [details] [diff] [review]
Remove -ansi -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE -DLINUX1_2

Compiles fine on Android.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

One of the reasons we use -ansi is that it forces compilation errors for 
comments in the c99 (c++) style, e.g. // comment, and also for variable 
definitions that occur elsewhere than at the beginning of a basic block,
that is, elsewhere than just after a '{'.  Since NSS must build on platforms 
whose compilers are not c99 compilers, we cannot allow // comments or 
mid-block variable definitions.  The idea is to get the compiler to tell
contributors about this error, so that code reviewers don't have to waste as 
much time on it, as they have in the past.  

Please find a solution that does not force code reviewers to have to go back 
to rejecting lots of contributions with unacceptable c99 code.

Comment 13

[Wan-Teh Chang]

Good point.  Note that -ansi only disallows C++ style // comments.
-ansi still allows mid-block variable declarations.

$ cat foo.c
int incr(int a)
{
    a++;
    int result = a;
    return result;
}
$ gcc -v
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --enable-languages=c,c++,fortran,objc,obj-c++,treelang --prefix=/usr --enable-shared --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --enable-nls --with-gxx-include-dir=/usr/include/c++/4.2 --program-suffix=-4.2 --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --enable-mpfr --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.2.4 (Ubuntu 4.2.4-1ubuntu4)
$ gcc -c -ansi -Wall foo.c
$

Michael, does -std=c89 work for Android?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Hmmm.  Note that 

{ a++; int result = a; return result; }

is equivalent to 

{  int result = ++a; return result; }

I wonder if that's relevant.  

How about this test:

{
char a[100];
memset(a, 0, sizeof a);
int result = a[2];
return result;
}

Comment 15

[Michael Wu [:mwu]]

Nope. -std=c89 really does seem identical to -ansi. However, -std=gnu89 does work, and doesn't define __STRICT_ANSI__. It is also the default if nothing is specified. Combined with -pedantic, it should print warnings on a large majority of code that isn't exactly C89.

So if we were to run your example with: gcc -c -ansi -Wall -pedantic foo.c, we get
foo.c: In function ‘incr’:
foo.c:4: warning: ISO C90 forbids mixed declarations and code

Comment 16

[Wan-Teh Chang]

Here are the C language features that have caused trouble
for us in the past.

1. C++ style // comments: I don't know which C compiler
   we're using still disallows this.

2. Mid-block variable declarations: disallowed by Visual
   C++ (see bug 275744 comment 89).

3. Commas following the last value in an enum type
   definition: disallowed by AIX compiler (see bug
   507482).

GCC's -ansi flag cannot prevent problems with #2 and #3,
as the bugs I cited show.  So its only value is in
preventing problems with #1.  But I don't know if #1 is
still an issue.

I can't find a GCC flag that turns off support for
C++ style // comments, so if Nelson insists on a compiler
flag that disallows C++ style comments, we'll have to
use Michael's original patch (with a comment to explain
the need for -ansi to be placed at that position).

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Does -std=gnu89 detect more than 1 of these?  

I think the compilers which impose the greatest of these restrictions are
the ones on HP-UX and AIX.  They also impose restrictions on initializers
of automatic arrays.  Anything we can do to make gcc users less likely to
produce code that upsets non-GCC compilers, and therefore less likely to 
be rejected by code review, will help us, a lot.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I don't understand -- stuff like // comments is very easy to see during code review; I guess it's nice if the compiler also rejects it, but it seems like an easy thing for reviewers to watch out for?

Comment 19

[Michael Wu [:mwu]]

Here's a piece of code that features all three problems:

enum foonum {
    ABC,
    DEF,
};

int incr(int a)
{
    // hello
    a = ABC;
    a++;
    int result = a;
    return result;
}

Compiling it with "gcc -c -Wall -std=gnu89 -pedantic test-foo.c" results in:
test-foo.c:8:5: warning: C++ style comments are not allowed in ISO C90
test-foo.c:8:5: warning: (this will be reported only once per input file)
test-foo.c:4: warning: comma at end of enumerator list
test-foo.c: In function ‘incr’:
test-foo.c:11: warning: ISO C90 forbids mixed declarations and code

All three problems are detected. To take it one step further, if you do "gcc -c -Wall -std=gnu89 -pedantic-errors test-foo.c", you get:
test-foo.c:4: error: comma at end of enumerator list
test-foo.c:8:5: error: C++ style comments are not allowed in ISO C90
test-foo.c:8:5: error: (this will be reported only once per input file)
test-foo.c: In function ‘incr’:
test-foo.c:11: error: ISO C90 forbids mixed declarations and code

Which is more strict than -ansi, without the __STRICT_ANSI__ definition. So as long as -std=gnu89 is placed before DSO_CFLAGS, or not specified at all (same thing), Android should be able to put in -std=gnu99 and make things work. I'll post a patch as soon as I verify things build on both android and glibc.

Comment 20

[Michael Wu [:mwu]]

Well, with -pedantic-errors, nothing really compiles. Casting void * to function pointers isn't supported (freebl), string lengths greater than 509 aren't supported (pkix/crlsel), returning in a void function isn't supported (freebl), 64bit (long long) isn't supported (mpl, sqlite), compiling empty C files isn't supported (dirent.c in dbm), etc.

Comment 21

[Michael Wu [:mwu]]

Attachment: Allow -ansi to be overridden by other CFLAGS, remove LINUX1_2

This is basically my first patch with the cleanups in the later patches and a comment about why -ansi is where it is. _POSIX_SOURCE, _BSD_SOURCE, and _XOPEN_SOURCE are also moved to the beginning to allow overrides if they cause problems in the future, though Android doesn't seem to care.

Comment 22

[Wan-Teh Chang]

Comment on attachment 421129 [details] [diff] [review]
Allow -ansi to be overridden by other CFLAGS, remove LINUX1_2

Michael, could you show me how you're going to override DSO_CFLAGS?
Can you override OS_REL_CFLAGS instead?  "DSO" implies the flags
are related to building shared libraries, but -ansi has nothing to
do with that.

I wonder we should add a new make variable, say LINUX_FEATURES_CFLAGS
or COMPILATION_ENVIRONMENT_CFLAGS, specifically for you to override -ansi.

Comment 23

[Michael Wu [:mwu]]

(In reply to comment #22)
> (From update of attachment 421129 [details] [diff] [review])
> Michael, could you show me how you're going to override DSO_CFLAGS?
> Can you override OS_REL_CFLAGS instead?  "DSO" implies the flags
> are related to building shared libraries, but -ansi has nothing to
> do with that.
> 
There's a variable in security/manager/Makefile.in named DEFAULT_GMAKE_FLAGS which contains all the overrides used in building NSS. For android, I currently have:

ifeq ($(OS_TARGET), Android)
DEFAULT_GMAKE_FLAGS += \
        OS_RELEASE="2.6" \
        OS_LIBS= \
        DSO_CFLAGS="$(CFLAGS) -std=gnu99" \
        DSO_LDOPTS="-shared $(LDFLAGS)" \
        $(NULL)
endif

-fPIC should probably also be in DSO_CFLAGS. The value assigned here cannot be easily changed by the makefile so OS_REL_CFLAGS+=-DLINUX2_0 (or whatever else would not work. 

DSO_CFLAGS looks like LINUX_FEATURES_CFLAGS or COMPILATION_ENVIRONMENT_CFLAGS right now since it seems to be used for compiling everything. I'd be fine with either renaming DSO_CFLAGS or adding a new variable that is expected to be overriden. What would you prefer?

Comment 24

[Wan-Teh Chang]

Comment on attachment 421129 [details] [diff] [review]
Allow -ansi to be overridden by other CFLAGS, remove LINUX1_2

r=wtc.

>+# bug 537829

Please mention comment 23, in which you described
exactly how you override -ansi using DSO_CFLAGS.

If we provide a new variable for you, for example,
STANDARDS_CFLAGS:

STANDARDS_CFLAGS = -ansi -D_XOPEN_SOURCE -D_POSIX_SOURCE -D_BSD_SOURCE
OS_CFLAGS        = $(STANDARDS_CFLAGS) $(DSO_CFLAGS) $(OS_REL_CFLAGS) $(ARCHFLAG) -Wall -Werror-implicit-function-declaration -Wno-switch -pipe -DLINUX -Dlinux -DHAVE_STRERROR

you'll override it in mozilla/security/manager/Makefile.in
like this:

ifeq ($(OS_TARGET), Android)
DEFAULT_GMAKE_FLAGS += \
       OS_RELEASE="2.6" \
       OS_LIBS= \
       STANDARDS_CFLAGS=-std=gnu99 \
       DSO_CFLAGS="$(CFLAGS)" \
       DSO_LDOPTS="-shared $(LDFLAGS)" \
       $(NULL)
endif

I believe you still need to override DSO_CFLAGS to pass
in the value of your CFLAGS.

Comment 25

[Michael Wu [:mwu]]

Attachment: Allow -ansi to be overridden by other CFLAGS, remove LINUX1_2, v2

Adds note about comment 23 in this bug.

Comment 26

[Michael Wu [:mwu]]

Thanks for the review!

Comment 27

[Wan-Teh Chang]

Attachment: Allow -ansi to be overridden by other CFLAGS, remove LINUX1_2, v3

I put the -ansi and *_SOURCE flags in a new STANDARDS_CFLAGS
make variable so that you can also override -ansi using
STANDARDS_CFLAGS.

I checked in the patch on the NSS trunk (NSS 3.12.6).

Checking in Linux.mk;
/cvsroot/mozilla/security/coreconf/Linux.mk,v  <--  Linux.mk
new revision: 1.43; previous revision: 1.42
done