684254 - prinet.h should include sys/endian.h on some Android NDK configurations in order to include htonl and friends

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[(no longer active)]

This is the NSPR spinoff from bug 684150.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Wan-Teh Chang]

Comment on attachment 557847 [details] [diff] [review]
Patch (v1)

Thanks for the patch.

NSPR public headers, including nsprpub/pr/include/prinet.h,
cannot depend on macros defined by configure.

Is there a compiler-predefined macro that we can test to
detect we are using Android NDK?

Comment 3

[(no longer active)]

(In reply to Wan-Teh Chang from comment #2)
> Comment on attachment 557847 [details] [diff] [review]
> Patch (v1)
> 
> Thanks for the patch.
> 
> NSPR public headers, including nsprpub/pr/include/prinet.h,
> cannot depend on macros defined by configure.
> 
> Is there a compiler-predefined macro that we can test to
> detect we are using Android NDK?

No, not really.  But aren't macros like XP_UNIX also defined by configure?

Comment 4

[Wan-Teh Chang]

Macros like XP_UNIX are defined by "prcpucfg.h" (the various
nsprpub/pr/include/md/_xxxos.cfg files).  They are redundantly
defined by configure.

Comment 5

[Wan-Teh Chang]

Please copy and paste the compilation error messages.
I can try to come up with a solution.

Comment 6

[(no longer active)]

(In reply to Ehsan Akhgari [:ehsan] from comment #0)
> This is the NSPR spinoff from bug 684150.

Sorry, this must have read bug 684139.

Here's the compilation error:

/Users/ehsanakhgari/Downloads/android-ndk-r6/toolchains/arm-linux-androideabi-4.4.3/prebuilt/darwin-x86/bin/arm-linux-androideabi-g++ -o nsPNGDecoder.o -c  -fvisibility=hidden -DMOZ_PNG_WRITE -DMOZ_PNG_READ  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux\" -DOSARCH=Linux -I/Users/ehsanakhgari/moz/tmp/modules/libpr0n/src/ -I/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/Users/ehsanakhgari/moz/tmp/obj-android/dist/include/nspr -I/Users/ehsanakhgari/moz/tmp/obj-android/dist/include/nss      -fPIC -I/Users/ehsanakhgari/Downloads/android-ndk-r6/platforms/android-5/arch-arm/usr/include -I/Users/ehsanakhgari/Downloads/android-ndk-r6/sources/cxx-stl/stlport/stlport  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -pedantic -Wno-long-long -mandroid -I/Users/ehsanakhgari/Downloads/android-ndk-r6/platforms/android-5/arch-arm/usr/include -fno-short-enums -fno-exceptions -fno-strict-aliasing -std=gnu++0x -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -ffunction-sections -fdata-sections -pipe  -DDEBUG -D_DEBUG -DTRACING -g  -I/Users/ehsanakhgari/Downloads/android-ndk-r6/platforms/android-5/arch-arm/usr/include -I/Users/ehsanakhgari/Downloads/android-ndk-r6/sources/cxx-stl/stlport/stlport  -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/nsPNGDecoder.pp /Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.cpp
In file included from /Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.cpp:44:
/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.h: In member function 'bool mozilla::imagelib::nsPNGDecoder::HasValidInfo() const':
/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.h:78: warning: 'png_info_struct::valid' is deprecated (declared at ../../../dist/include/png.h:657)
/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.h:78: warning: 'png_info_struct::valid' is deprecated (declared at ../../../dist/include/png.h:657)
/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.h: In member function 'PRInt32 mozilla::imagelib::nsPNGDecoder::GetPixelDepth() const':
/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.h:87: warning: 'png_info_struct::pixel_depth' is deprecated (declared at ../../../dist/include/png.h:682)
/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.h:87: warning: 'png_info_struct::pixel_depth' is deprecated (declared at ../../../dist/include/png.h:682)
/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.cpp: In static member function 'static void mozilla::imagelib::nsPNGDecoder::row_callback(png_struct*, png_byte*, png_uint_32, int)':
/Users/ehsanakhgari/moz/tmp/modules/libpr0n/decoders/nsPNGDecoder.cpp:758: error: 'ntohl' was not declared in this scope
make: *** [nsPNGDecoder.o] Error 1

Comment 7

[Wan-Teh Chang]

There are three possible solutions.

1. Change modules/libpr0n/decoders/nsPNGDecoder.cpp to include "prnetdb.h"
and use PR_ntohl instead of ntohl.

Note: PR_ntohl is a function, whereas ntohl may be a macro or compiler
intrinsic.  If this is not in peformance-critical code, the function
call overhead of PR_ntohl should be OK.

2. Change modules/libpr0n/decoders/nsPNGDecoder.cpp to include
<sys/endian.h> for Android NDK, with a corresponding configure test.

3. Figure out how to detect these Android NDK configurations using
compiler-predefined macros.

Note: it was a historical mistake for "prinet.h" to provide the
definitions of ntohl, etc. to NSPR clients.  In hindsight, "prinet.h"
should have been an NSPR internal header.

I will try to support this, but if we cannot detect these Android NDK
using compiler-predefined macros, please implement Solution 1 or 2.

Comment 8

[Wan-Teh Chang]

I just remembered this bug is related to bug 411055, and
the "Hybrid approach" patch in bug 406580.

So, I recommend Solution 3 or 2.  Solution 2 should modify
gfx/thebes/gfxColor.h instead:

2. Change gfx/thebes/gfxColor.h to include <sys/endian.h>
for Android NDK, with a corresponding configure test.

The Single Unix Specification says including <arpa/inet.h>
should get ntohl().  "prinet.h" includes <arpa/inet.h> on
all Unix platforms, so this seems like an Android NDK's
incompatibility with Unix.  Can you attach the <arpa/inet.h>
file from Android NDK?

http://predef.sourceforge.net/preos.html#sec4 says _ANDROID__
is a gcc predefined macro for Android.  Can we use that to
implement Solution 3?

Comment 9

[Wan-Teh Chang]

My web search shows that if you use NDK r5 or later,
you should be able to implement Solution 3 by testing
__ANDROID__:
http://groups.google.com/group/android-ndk/browse_thread/thread/c93b0cedc6a6fdd9
http://groups.google.com/group/android-ndk/browse_thread/thread/9c1922c3b3e8e6e6

Another hack is to include <sys/stat.h>.  It seems that Android
NDK's <sys/stat.h> includes <endian.h>, which includes <sys/endian.h>.
This may be why NSPR itself (nsprpub/pr/src/misc/prnetdb.c, which
calls ntohl()) can get by without including <sys/endian.h> because
NSPR's internal headers include <sys/stat.h>.  But I think it's
better to implement Solution 3 with __ANDROID__.

Comment 10

[(no longer active)]

Attachment: Patch (v2)

Implementing solution 3.

Comment 11

[Wan-Teh Chang]

Attachment: Patch (v3) by Ehsan Akhgari

Patch checked in on the NSPR trunk (NSPR 4.9).

Checking in prinet.h;
/cvsroot/mozilla/nsprpub/pr/include/prinet.h,v  <--  prinet.h
new revision: 3.18; previous revision: 3.17
done

Comment 12

[(no longer active)]

Thanks a lot for committing this, Wan-Teh!

How should we get this fix on mozilla-central?

Comment 13

[(no longer active)]

Pushed to mozilla-central in the latest NSPR update: https://hg.mozilla.org/mozilla-central/rev/7e25caedd620