794510 - Build with NO_NSPR_10_SUPPORT by default to remove the dependency on protypes.h

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(no longer active)]

protypes.h is giving us pains all over the place.  Ms2ger mentioned today that we need to build with NO_NSPR_10_SUPPORT to make sure that prtypes.h does not include that header.

So here's what needs to happen here:
1. Modify configure.in (and perhaps js/src/configure.in) to add NO_NSPR_10_SUPPORT to our default defines on all platforms.
2. Remove all of the places where we define NO_NSPR_10_SUPPORT in individual makefiles.
3. Remove the places where we force undef NO_NSPR_10_SUPPORT.
4. Beat the code enough to build on all platforms.

I'm willing to mentor someone in doing this, but this is not really a good first bug, some previous Gecko hacking experience helps a lot!

Comment 1

[(no longer active)]

Isaac, I think you will be an excellent candidate for this bug.  Do you wanna take it on?

Comment 2

[Isaac Aggrey [:isaac]]

(In reply to Ehsan Akhgari [:ehsan] from comment #1)
> Isaac, I think you will be an excellent candidate for this bug.  Do you
> wanna take it on?

Sounds interesting, I will!

Comment 3

[Isaac Aggrey [:isaac]]

I'm not familiar with autoconf and the build system, but I attempted to read some MDN docs and the code itself to get an idea. Is the strategy for each step something like the following:

Step 1) Modify configure.in (do we know for sure if js/src/configure.in is necessary? I couldn't tell if also needed to add there based on bug 605133)

dnl ========================================================
dnl = Build with NO_NSPR_10_SUPPORT
dnl ========================================================
if test -n "$NO_NSPR_10_SUPPORT"; then
    AC_DEFINE(NO_NSPR_10_SUPPORT)
fi

Step 2) Remove all places where we define NO_NSPR_10_SUPPORT in individual places

Remove these: https://mxr.mozilla.org/mozilla-central/search?string=NO_NSPR_10_SUPPORT&find=Makefile.in&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

Step 3) Remove the places where we force undef NO_NSPR_10_SUPPORT

Remove these: https://mxr.mozilla.org/mozilla-central/search?string=%23undef+NO_NSPR_10_SUPPORT&regexp=1&find=&findi=&filter=%5E%5B%5E%5C0%5D*%5Cn%24&hitlimit=&tree=mozilla-central

Step 4) Beat the code enough to build on all platforms.

Beat it. Just beat it. (sorry, couldn't resist)

Comment 4

[(no longer active)]

(In reply to Isaac Aggrey [:isaac] from comment #3)
> I'm not familiar with autoconf and the build system, but I attempted to read
> some MDN docs and the code itself to get an idea. Is the strategy for each
> step something like the following:
> 
> Step 1) Modify configure.in (do we know for sure if js/src/configure.in is
> necessary? I couldn't tell if also needed to add there based on bug 605133)
> 
> dnl ========================================================
> dnl = Build with NO_NSPR_10_SUPPORT
> dnl ========================================================
> if test -n "$NO_NSPR_10_SUPPORT"; then
>     AC_DEFINE(NO_NSPR_10_SUPPORT)
> fi

No, this is not what I had in mind.  I was thinking of adding this flag to the list of compiler preprocessor definitions using the -D flag.  So you should roughly do something like this:

Add an m4 file to build/autoconfig, which adds the preprocessor macro, roughly like this:

CFLAGS="$CFLAGS -DNO_NSPR_10_SUPPORT"
CXXFLAGS="$CXXFLAGS -DNO_NSPR_10_SUPPORT"

and then include that m4 in configure.in and js/src/configure.in.

I'm sure the build system peers will correct me if I'm wrong here.

> Step 2) Remove all places where we define NO_NSPR_10_SUPPORT in individual
> places
> 
> Remove these:
> https://mxr.mozilla.org/mozilla-central/
> search?string=NO_NSPR_10_SUPPORT&find=Makefile.
> in&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

Precisely.

> Step 3) Remove the places where we force undef NO_NSPR_10_SUPPORT
> 
> Remove these:
> https://mxr.mozilla.org/mozilla-central/
> search?string=%23undef+NO_NSPR_10_SUPPORT&regexp=1&find=&findi=&filter=%5E%5B
> %5E%5C0%5D*%5Cn%24&hitlimit=&tree=mozilla-central

Yep!

> Step 4) Beat the code enough to build on all platforms.
> 
> Beat it. Just beat it. (sorry, couldn't resist)

Well, I'm sure you have to beat it hard!  :-)

Comment 5

[Isaac Aggrey [:isaac]]

The first code beating needs to address the old compatibility functions, PR_FormatTime and PR_FormatTimeUSEnglish defined here: https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#258

and used in code here: https://mxr.mozilla.org/mozilla-central/search?string=PR_FormatTime

...which causes "not declared in this scope" errors (obviously).

PR_FormatTime has the same semantics as strftime except it takes in PRExplodedTime rather than ANSI C's tm type: http://en.cppreference.com/w/c/chrono/tm

PRExplodedTime's differences are described here: https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#60

I'm not sure where to go from here. Since we want NO_NSPR_10_SUPPORT, are we trying to go to strftime? Or do we need to replace those functions with new functions? Or do something else?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Something that might work is creating a new header in m-c that's basically

#undef NO_NSPR_10_SUPPORT
#include "prtime.h"
#define NO_NSPR_10_SUPPORT

and use that instead.

Comment 7

[(no longer active)]

Yeah, I was about to suggest that we should just change the callers to use strftime directly, and then I looked at the implementation of PR_FormatTime <https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/misc/prtime.c#1675> and that's pretty much what it does.  So I don't think there's too much point in converting those callers to use strftime in this bug.  So, let's go with Ms2ger's suggestion and add a header named NSPRFormatTime.h or something, and put some comments in there on why this is needed.

Comment 8

[Isaac Aggrey [:isaac]]

I ran into the first issue with the PR_FormatTime functions by manually looking for it, but I can't seem to actually get configure.in to use the m4 file that has the compiler flags, I've tried using: m4_include(filename), adding aclocal.m4 list of builtin m4 calls

How do you include an m4 file to configure.in?

(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> let's go with Ms2ger's suggestion and add a header named NSPRFormatTime.h or
> something, and put some comments in there on why this is needed.

Where should the header be added? I can't seem to find any documentation on where headers go.

Also, I'm not sure I understand the big picture (bug 796941) correctly since its goal is to remove usage of prtypes.h. It's OK we include prtime.h (which has a dependency on prtypes.h) in this case?

Comment 9

[(no longer active)]

(In reply to comment #8)
> I ran into the first issue with the PR_FormatTime functions by manually looking
> for it, but I can't seem to actually get configure.in to use the m4 file that
> has the compiler flags, I've tried using: m4_include(filename), adding
> aclocal.m4 list of builtin m4 calls
> 
> How do you include an m4 file to configure.in?

I'm not really sure..  glandium?

> (In reply to Ehsan Akhgari [:ehsan] from comment #7)
> > let's go with Ms2ger's suggestion and add a header named NSPRFormatTime.h or
> > something, and put some comments in there on why this is needed.
> 
> Where should the header be added? I can't seem to find any documentation on
> where headers go.

You should be able to add it to the EXPORTS variable in Makefile.in.

> Also, I'm not sure I understand the big picture (bug 796941) correctly since
> its goal is to remove usage of prtypes.h. It's OK we include prtime.h (which
> has a dependency on prtypes.h) in this case?

For now, it's OK.  I still don't have a concrete plan on how to handle other NSPR headers which depend on prtypes.h, but the more prtypes.h inclusions we can remove, the merrier!

Comment 10

[Mike Hommey [:glandium]]

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> (In reply to comment #8)
> > I ran into the first issue with the PR_FormatTime functions by manually looking
> > for it, but I can't seem to actually get configure.in to use the m4 file that
> > has the compiler flags, I've tried using: m4_include(filename), adding
> > aclocal.m4 list of builtin m4 calls
> > 
> > How do you include an m4 file to configure.in?
> 
> I'm not really sure..  glandium?

Adding a builtin(include, build/autoconf/yourfile.m4)dnl
line to aclocal.m4.

Comment 11

[Isaac Aggrey [:isaac]]

(In reply to Mike Hommey [:glandium] from comment #10)
> Adding a builtin(include, build/autoconf/yourfile.m4)dnl
> line to aclocal.m4.

Thank you, I must not have rebuilt correctly the first time because I tried that and it didn't seem like it was working.

> (In reply to Ehsan Akhgari [:ehsan] from comment #9)
> For now, it's OK.  I still don't have a concrete plan on how to handle other
> NSPR headers which depend on prtypes.h, but the more prtypes.h inclusions we
> can remove, the merrier!

Now that I'm properly building with NO_NSPR_10_SUPPORT, I'm running into a lot of the obsolete uintN typedefs from protypes.h. Is it correct to assume that I'm replacing those with standard int types?

Also, are there any other directories from upstream projects that I should be aware of so I don't touch those files? dbm, security/nss, and nsprpub directories...is that it?

Comment 12

[(no longer active)]

(In reply to comment #11)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #9)
> > For now, it's OK.  I still don't have a concrete plan on how to handle other
> > NSPR headers which depend on prtypes.h, but the more prtypes.h inclusions we
> > can remove, the merrier!
> 
> Now that I'm properly building with NO_NSPR_10_SUPPORT, I'm running into a lot
> of the obsolete uintN typedefs from protypes.h. Is it correct to assume that
> I'm replacing those with standard int types?

Yes.

Are there a lot of those?  If yes, maybe we should consider rewriting them using a script.  Do you have a list of the protypes being used?

> Also, are there any other directories from upstream projects that I should be
> aware of so I don't touch those files? dbm, security/nss, and nsprpub
> directories...is that it?

Yeah I think so.  We can pretty much modify everything else.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

A script: https://bug787933.bugzilla.mozilla.org/attachment.cgi?id=657850

I assume you'll need to twiddle with the find command at the start to catch the correct dirs.

Comment 14

[(no longer active)]

(I'd appreciate if you can put the type name changes in a separate patch!)

Comment 15

[Isaac Aggrey [:isaac]]

(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> Are there a lot of those?  If yes, maybe we should consider rewriting them
> using a script.  Do you have a list of the protypes being used?

I was changing them by hand early on, but after looking through MXR there are enough to warrant the use of a script.

> > Also, are there any other directories from upstream projects that I should be
> > aware of so I don't touch those files? dbm, security/nss, and nsprpub
> > directories...is that it?
> 
> Yeah I think so.  We can pretty much modify everything else.

Gotcha, just what I needed to know.

(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> (I'd appreciate if you can put the type name changes in a separate patch!)

I actually have five separate patches right now. :-)

(1) Building with NO_NSPR_10_SUPPORT and removing the file specific NO_NSPR_10_SUPPORT inclusions
(2) the integer type name changes
(3) the creation (and subsequent usage) of NSPRFormatTime.h
(4) the creation (and subsequent usage) of NSPRLog.h (similar issue as (3) for prlog.h)
(5) prarena.h to plarena.h type changes

QUESTION #1

Regarding (3), different files within netwerk's subdirectories require the header, so I placed it in netwerk. However, it seems like headers shouldn't be placed there...what's the proper way to reuse a header in different subdirectories?

QUESTION #1.5

Similar question with (4) except usage of NSPRLog.h is in hal and dom/camera

QUESTION #2

I'm hitting the prarena.h -> plarena.h definitions now: https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/obsolete/protypes.h#149

I've only hit one so far, and I simply replaced it with the correct plarena.h type. Is this right?

If there are significantly more, I'll use a script.

Comment 16

[Isaac Aggrey [:isaac]]

Attachment: Script to replace obsolete int types with stdint ones

Comment 17

[(no longer active)]

(In reply to comment #15)
> (In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > Are there a lot of those?  If yes, maybe we should consider rewriting them
> > using a script.  Do you have a list of the protypes being used?
> 
> I was changing them by hand early on, but after looking through MXR there are
> enough to warrant the use of a script.

Ouch!  Well, thanks for not getting disappointed by the amount of stupidity going on in our code.  :-)

> (In reply to Ehsan Akhgari [:ehsan] from comment #14)
> > (I'd appreciate if you can put the type name changes in a separate patch!)
> 
> I actually have five separate patches right now. :-)
> 
> (1) Building with NO_NSPR_10_SUPPORT and removing the file specific
> NO_NSPR_10_SUPPORT inclusions
> (2) the integer type name changes
> (3) the creation (and subsequent usage) of NSPRFormatTime.h
> (4) the creation (and subsequent usage) of NSPRLog.h (similar issue as (3) for
> prlog.h)
> (5) prarena.h to plarena.h type changes

Heh, well that's great!

> QUESTION #1
> 
> Regarding (3), different files within netwerk's subdirectories require the
> header, so I placed it in netwerk. However, it seems like headers shouldn't be
> placed there...what's the proper way to reuse a header in different
> subdirectories?

So, netwerk/base/public is sort of a dumpster for the stuff that netwerk relies on but we don't know where else to put them.  But I think we can avoid putting these headers there in this case.  Can't you just put them somewhere in xpcom?  Such as xpcom/base?

> QUESTION #1.5
> 
> Similar question with (4) except usage of NSPRLog.h is in hal and dom/camera

xpcom/base again?

> QUESTION #2
> 
> I'm hitting the prarena.h -> plarena.h definitions now:
> https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/obsolete/protypes.h#149

Yikes!

> I've only hit one so far, and I simply replaced it with the correct plarena.h
> type. Is this right?

Absolutely.

> If there are significantly more, I'll use a script.

Yeah makes sense.  Thanks!

Comment 18

[Isaac Aggrey [:isaac]]

Attachment: Script to replace obsolete int types with stdint types

This version avoids clobbering what seem like other upstream or third party projects in mozilla-central that caused build issues when touched.

Comment 19

[Isaac Aggrey [:isaac]]

Attachment: Part 1: Build with NO_NSPR_10_SUPPORT

Comment 20

[Isaac Aggrey [:isaac]]

Attachment: Part 2: Use stdint types

Comment 21

[Isaac Aggrey [:isaac]]

Attachment: Part 3: Use new header NSPRFormatTime.h when temp undef of NO_NSPR_10_SUPPORT required

I'm not sure if the comment in NSPRFormatTime.h is sufficiently descriptive.

Comment 22

[Isaac Aggrey [:isaac]]

Attachment: Part 4: Use new header NSPRLog.h when temp undef of NO_NSPR_10_SUPPORT required

Same here on comment in NSPRLog.h

Comment 23

[Isaac Aggrey [:isaac]]

Attachment: Part 5: Use plarena.h types

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 670288 [details] [diff] [review]
Part 2: Use stdint types

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

Thanks a lot! There's quite a bit of third party code we shouldn't touch, though :/

::: content/base/src/nsGkAtomList.h
@@ -454,5 @@
>  GK_ATOM(insertafter, "insertafter")
>  GK_ATOM(insertbefore, "insertbefore")
>  GK_ATOM(instanceOf, "instanceOf")
> -GK_ATOM(int32, "int32")
> -GK_ATOM(int64, "int64")

This change is wrong, I'm afraid.

::: content/xul/templates/src/nsXULTemplateQueryProcessorStorage.cpp
@@ -325,5 @@
>                  parameterCount++;
>              }
>  
>              static nsIContent::AttrValuesArray sTypeValues[] =
> -                { &nsGkAtoms::int32, &nsGkAtoms::integer, &nsGkAtoms::int64,

As is this. Looks like this is the only one, though.

::: db/sqlite3/src/sqlite3.c
@@ -57265,1 @@
>  ** there are reports that windows throws an expection

This is imported code too.

::: dom/plugins/base/android/android_npapi.h
@@ +1036,5 @@
>  
> +typedef int32_t   int32_t;
> +typedef uint32_t uint32_t;
> +typedef int16_t   int16_t;
> +typedef uint16_t uint16_t;

This is starting to look a little silly now :)

::: gfx/angle/include/KHR/khrplatform.h
@@ -190,5 @@
>  
>  #elif 0
>  
>  /*
> - * Hypothetical platform with no float or int64 support

Upstream.

::: gfx/angle/src/compiler/MapLongVariableNames.cpp
@@ -12,5 @@
>  TString mapLongName(int id, const TString& name, bool isGlobal)
>  {
>      ASSERT(name.size() > MAX_SHORTENED_IDENTIFIER_SIZE);
>      TStringStream stream;
> -    uint64 hash = SpookyHash::Hash64(name.data(), name.length(), 0);

Leave this and the other two spooky files alone; they have their own definition, silly as that may be.

::: gfx/graphite2/src/Bidi.cpp
@@ -296,5 @@
>  { /*ret*/xxx, xxx, xxx, xxx, xxE, xxR, xxE, xxN, xxN, xxE, /* ET following re           */ },
>  { /*let*/xxx, xxx, xxx, xxx, xxL, xxR, xxL, xxN, xxN, xxL, /* ET following le           */ },
>  };
>  
> -inline uint8    GetDeferredType(bidi_action a)          { return (a >> 4) & 0xF; }

graphite2 has its own definition as well; please leave these changes out.

::: gfx/skia/src/images/SkImageDecoder_libbmp.cpp
@@ -54,5 @@
>      // we don't copy the bitmap, just remember the pointer
>      SkBmpDecoderCallback(bool justBounds) : fJustBounds(justBounds) {}
>  
>      // override from BmpDecoderCallback
> -    virtual uint8* SetSize(int width, int height) {

Doesn't look like this code is actually being compiled at the moment... It's upstream, though, so I'd leave gfx/skia alone as well.

::: gfx/skia/src/images/bmpdecoderhelper.h
@@ +18,5 @@
>  #include <limits.h>
>  #define DISALLOW_EVIL_CONSTRUCTORS(name)
>  #define CHECK(predicate)  SkASSERT(predicate)
> +typedef uint8_t uint8_t;
> +typedef uint32_t uint32_t;

Mm...

::: gfx/ycbcr/chromium_types.h
@@ +17,1 @@
>  #endif

Just remove this block.

::: intl/uconv/tools/umaptable.c
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include <stdio.h>
> +typedef short int16_t;
> +typedef unsigned short uint16_t;

Include mozilla/StandardInteger.h instead.

::: js/public/LegacyIntTypes.h
@@ +28,5 @@
>  
> +typedef uint8_t uint8_t;
> +typedef uint16_t uint16_t;
> +typedef uint32_t uint32_t;
> +typedef uint64_t uint64_t;

Leave these alone.

::: js/src/ctypes/CTypes.cpp
@@ -7230,5 @@
>    }
>  
>    int64_t i = 0;
>    if (!jsvalToBigInteger(cx, args[0], true, &i))
> -    return TypeError(cx, "int64", args[0]);

And this

::: js/src/ion/IonTypes.h
@@ +19,5 @@
> +typedef int32_t int32_t;
> +typedef uint16_t uint16_t;
> +typedef int16_t int16_t;
> +typedef uint8_t uint8_t;
> +typedef int8_t int8_t;

Remove these

::: js/src/methodjit/PolyIC.cpp
@@ -2921,5 @@
>  {
>      if (!objval.isObject())
>          return disable(f, "primitive lval");
>      if (!idval.isInt32())
> -        return disable(f, "non-int32 key");

You can leave this

::: media/libopus/include/opus_types.h
@@ -91,2 @@
>     typedef u_int16_t opus_uint16;
>     typedef int32_t opus_int32;

Shouldn't touch media/libopus either.

::: media/libtremor/lib/tremor_block.c
@@ -403,5 @@
>  	   then spec says the end is cut, not beginning */
>  	long extra=b->sample_count-vb->granulepos;
>  
>          /* we use ogg_int64_t for granule positions because a
> -           uint64 isn't universally available.  Unfortunately,

No need for these changes.

::: media/webrtc/signaling/src/sipcc/core/includes/embedded.h
@@ -54,4 @@
>  
> -typedef unsigned char uint8;
> -typedef unsigned short uint16;
> -typedef unsigned int uint32;

I guess you get to ignore media/webrtc/signaling/src/sipcc too.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_os_defs.h
@@ -61,2 @@
>  typedef uint32_t   u32;
> -typedef uint32_t   uint32;

Mad, I tell you, they're all mad.

::: media/webrtc/trunk/src/modules/audio_processing/test/unit_test.cc
@@ -244,5 @@
>  void MixStereoToMono(const int16_t* stereo,
>                       int16_t* mono,
>                       int samples_per_channel) {
>    for (int i = 0; i < samples_per_channel; i++) {
> -    int32_t int32 = (static_cast<int32_t>(stereo[i * 2]) +

Heh. No change needed here.

::: media/webrtc/trunk/src/modules/audio_processing/test/unpack.cc
@@ -34,5 @@
>  DEFINE_string(reverse_file, "reverse.pcm",
>                "The name of the reverse input stream file.");
> -DEFINE_string(delay_file, "delay.int32", "The name of the delay file.");
> -DEFINE_string(drift_file, "drift.int32", "The name of the drift file.");
> -DEFINE_string(level_file, "level.int32", "The name of the level file.");

Might be better not to touch these either

::: media/webrtc/trunk/third_party/libyuv/include/libyuv/basic_types.h
@@ +30,5 @@
>  #endif
> +typedef uint16_t uint16_t;
> +typedef int16_t  int16_t;
> +typedef uint8_t  uint8_t;
> +typedef int8_t   int8_t;

Oh dear, what a mess.

::: mfbt/double-conversion/diy-fp.h
@@ -32,5 @@
>  
>  namespace double_conversion {
>  
>  // This "Do It Yourself Floating Point" class implements a floating-point number
> -// with a uint64 significand and an int exponent. Normalized DiyFp numbers will

Upstream too, I think.

Comment 25

[Mozilla RelEng Bot]

Try run for bc65f8e1b8c9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bc65f8e1b8c9
Results (out of 16 total builds):
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bc65f8e1b8c9

Comment 26

[Isaac Aggrey [:isaac]]

(In reply to :Ms2ger from comment #24)
> Comment on attachment 670288 [details] [diff] [review]
>
> Thanks a lot! There's quite a bit of third party code we shouldn't touch,
> though :/

Grr, I knew I'd get bitten by this...there should be clearer docs on what shouldn't be touched. :-/

In any case, thank you for looking over Part 2! I also caught some of the sillier mistakes after the fact...I still need to learn to look over my code closely, especially when scripts are involved.

Comment 27

[Isaac Aggrey [:isaac]]

Attachment: Part 2: Use stdint types

Incorporated Ms2ger's review

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

(In reply to :Ms2ger from comment #24)
> Comment on attachment 670288 [details] [diff] [review]
> Part 2: Use stdint types
> 
> Review of attachment 670288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks a lot! There's quite a bit of third party code we shouldn't touch,
> though :/

Isaac makes a good point; we should clearly mark imported code, both for our own purposes and for scripts.  Biesi and I were commiserating about the lack of a standard for this ("third_party/blah" such as netwerk/third_party/sctp instead of netwerk/sctp/src might be one way to do it, or put them ALL in a top-level third-party directory, or add a file in the root of a third-party directory (IMPORTED, with info on where/how/etc), ...)


> ::: media/webrtc/signaling/src/sipcc/core/includes/embedded.h
> @@ -54,4 @@
> >  
> > -typedef unsigned char uint8;
> > -typedef unsigned short uint16;
> > -typedef unsigned int uint32;
> 
> I guess you get to ignore media/webrtc/signaling/src/sipcc too.

Yup.
 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_os_defs.h
> @@ -61,2 @@
> >  typedef uint32_t   u32;
> > -typedef uint32_t   uint32;
> 
> Mad, I tell you, they're all mad.

certainly

> ::: media/webrtc/trunk/src/modules/audio_processing/test/unit_test.cc
> @@ -244,5 @@
> >  void MixStereoToMono(const int16_t* stereo,
> >                       int16_t* mono,
> >                       int samples_per_channel) {
> >    for (int i = 0; i < samples_per_channel; i++) {
> > -    int32_t int32 = (static_cast<int32_t>(stereo[i * 2]) +
> 
> Heh. No change needed here.

media/webrtc/trunk is all imported code as well.

> ::: media/webrtc/trunk/third_party/libyuv/include/libyuv/basic_types.h
> @@ +30,5 @@
> >  #endif
> > +typedef uint16_t uint16_t;
> > +typedef int16_t  int16_t;
> > +typedef uint8_t  uint8_t;
> > +typedef int8_t   int8_t;
> 
> Oh dear, what a mess.

Yeah, imported code in imported code....  We'll likely move it to a direct import at some point.  This one we had to modify to deal with code that (indirectly) included protypes.h and then included libyuv.h.  If the protypes.h goes away, then we can back out the evil changes in libyuv.

\o/

Comment 29

[(no longer active)]

(In reply to comment #28)
> (In reply to :Ms2ger from comment #24)
> > Comment on attachment 670288 [details] [diff] [review]
> > Part 2: Use stdint types
> > 
> > Review of attachment 670288 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks a lot! There's quite a bit of third party code we shouldn't touch,
> > though :/
> 
> Isaac makes a good point; we should clearly mark imported code, both for our
> own purposes and for scripts.  Biesi and I were commiserating about the lack of
> a standard for this ("third_party/blah" such as netwerk/third_party/sctp
> instead of netwerk/sctp/src might be one way to do it, or put them ALL in a
> top-level third-party directory, or add a file in the root of a third-party
> directory (IMPORTED, with info on where/how/etc), ...)

Well, marking imported code is definitely helpful, but really we should have a clear process on how to modify the imported code.  It is entirely unreasonable to render ourselves unable to modify our imported code (just look at the current situation with NSPR which causes developers to go through huge pain in order to work around things which would be very simply dealt with if only we had the option of fixing NSPR...).  We currently do a very good job in some of the projects (see angle for example: http://mxr.mozilla.org/mozilla-central/source/gfx/angle/) and in some other cases we do an extremely poor job (case in point: nsprpub/!).  Really, whoever maintains a given imported code should come up with a clear process on how to take patches to that code and either try to upstream it if it makes sense or maintain a local patch to assist updates to the imported code in the future.

Comment 30

[(no longer active)]

Comment on attachment 670286 [details] [diff] [review]
Part 1: Build with NO_NSPR_10_SUPPORT

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

This looks good to me, but I'd like glandium to take a look as well since this is mostly build-system changes.

Comment 31

[(no longer active)]

Comment on attachment 670290 [details] [diff] [review]
Part 3:  Use new header NSPRFormatTime.h when temp undef of NO_NSPR_10_SUPPORT required

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

Minusing for the Makefile changes as noted below.  The .cpp file changes look good.

::: netwerk/base/src/Makefile.in
@@ +18,5 @@
>  
>  EXPORTS = \
>  		nsMIMEInputStream.h \
>  		nsURLHelper.h \
> +		$(topsrcdir)/xpcom/base/NSPRFormatTime.h \

Hmm, you should not add the file to the EXPORTS variable here and in the other Makefiles in this patch.  You should do that in xpcom/base/Makefile.h.  What that does is to put the header in objdir/dist/include which means that everyone can #include it.  Sorry if I was not clear enough.

Comment 32

[(no longer active)]

Comment on attachment 670291 [details] [diff] [review]
Part 4:  Use new header NSPRLog.h when temp undef of NO_NSPR_10_SUPPORT required

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

Minusing for the same reason as part 3.

Comment 33

[Isaac Aggrey [:isaac]]

Attachment: Part 3: Use new header NSPRFormatTime.h when temp undef of NO_NSPR_10_SUPPORT required

Comment 34

[Isaac Aggrey [:isaac]]

Attachment: Part 4: Use new header NSPRLog.h when temp undef of NO_NSPR_10_SUPPORT required

Comment 35

[Isaac Aggrey [:isaac]]

Attachment: Part 2: Use stdint types

No longer touching media/webrtc or netwerk/sctp

Comment 36

[(no longer active)]

Comment on attachment 670335 [details] [diff] [review]
Part 2: Use stdint types

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

So I talked to the IonMonkey folks.  They're not really comfortable with doing large scale modifications to their code base as it makes it harder for them to backport fixes to aurora etc.  that's sort of fine for our purposes, since the define their own types in IonTypes.h, but it unfortunately means that you need to modify your patch again!  Can you please remove the stuff from js/src/ion from your patch, please?  The upside is that it makes your patch a lot smaller.  :-)  Also, please remove the hunks mentioned in comment 28 as well.

Thanks!

Comment 37

[(no longer active)]

Comment on attachment 670478 [details] [diff] [review]
Part 2: Use stdint types

Oh, sorry for not providing feedback on the previous version in time...

Comment 38

[Isaac Aggrey [:isaac]]

Attachment: Part 2: Use stdint types

No longer touching js/src/ion or netwerk/srtp

Comment 39

[Isaac Aggrey [:isaac]]

Attachment: Part 2: Use stdint types

Let's try that again...

Comment 40

[(no longer active)]

Comment on attachment 670606 [details] [diff] [review]
Part 2: Use stdint types

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

Looks good!

Comment 41

[(no longer active)]

Have you tested this stuff on the try server?  I'd like to get them landed sooner than later because these patches will bitrot fast!

Comment 42

[Mozilla RelEng Bot]

Try run for e4707365d7a2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e4707365d7a2
Results (out of 16 total builds):
    exception: 5
    failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e4707365d7a2

Comment 43

[Isaac Aggrey [:isaac]]

I'm not sure why my push to try didn't throw in Part 1 since it's in my mq, but it seems like there are more build issues that may be unrelated and symptoms of other deficiencies in my patches (strangely builds finish on my Linux system).

Comment 44

[Isaac Aggrey [:isaac]]

Try results: https://tbpl.mozilla.org/?tree=Try&rev=f2f546c568ba

It seems like Linux builds are successful, but the other platforms have a variety of problems:

* Android: appears to use some security/nss code that relies on prarena.h types that are not defined without protypes.h

* OS X: need to add "mozilla/StandardInteger.h" to nsCocoaFeatures.h (maybe others?)

* Windows: not sure...

* ARM: not sure here either...something about Python.h not being found

Comment 45

[Wan-Teh Chang]

(In reply to :Ms2ger from comment #6)
> Something that might work is creating a new header in m-c that's basically
> 
> #undef NO_NSPR_10_SUPPORT
> #include "prtime.h"
> #define NO_NSPR_10_SUPPORT
> 
> and use that instead.

Can you explain why the
  #undef NO_NSPR_10_SUPPORT
line is necessary? Thanks.

Comment 46

[Isaac Aggrey [:isaac]]

(In reply to Wan-Teh Chang from comment #45)
> Can you explain why the
>   #undef NO_NSPR_10_SUPPORT
> line is necessary? Thanks.

Both prtime.h and prlog.h have some compatibility functions that some files depend on which are not defined with NO_NSPR_10_SUPPORT.

See prtime.h: https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#258

See prlog.h: https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prlog.h#190

Comment 47

[Wan-Teh Chang]

Thanks for the answer.

prtime: we may be able to move those two functions outside
#ifndef NO_NSPR_10_SUPPORT. PR_FormatTimeUSEnglish is
definitely OK because it is entirely implemented by NSPR.
I need to think more about PR_FormatTime.

prlog.h: it seems that only one of the three macros is used by
Mozilla, and only in three files:

https://mxr.mozilla.org/mozilla-central/ident?i=PR_LOG_BEGIN
https://mxr.mozilla.org/mozilla-central/ident?i=PR_LOG_END
https://mxr.mozilla.org/mozilla-central/ident?i=PR_LOG_DEFINE

We should replace PR_LOG_DEFINE with PR_NewLogModule in those
three files.

Comment 48

[Isaac Aggrey [:isaac]]

(In reply to Wan-Teh Chang from comment #47)
> prtime: we may be able to move those two functions outside
> #ifndef NO_NSPR_10_SUPPORT. PR_FormatTimeUSEnglish is
> definitely OK because it is entirely implemented by NSPR.
> I need to think more about PR_FormatTime.

OK, thank you for looking into that. I'm not familiar with NSPR, so I appreciate any guidance on what the best approach forward is.

> prlog.h: it seems that only one of the three macros is used by
> Mozilla, and only in three files:

Good point, there isn't a good reason to go that route for prlog.h. I imagine using PR_NewLogModule directly is the way we want to go, so I'll attach an alternative patch in a bit -- especially since I'd touch less files that way.

Comment 49

[Isaac Aggrey [:isaac]]

Attachment: Part 4: Use PR_NewLogModule instead of PR_LOG_DEFINE

With this patch, build is still successful on my Linux machine (and presumably Linux in general).

Some advice on the other platform build errors are wanted (see comment 44), since I imagine it's not good etiquette to throw patches that may not work at the try server. :-)

Comment 50

[(no longer active)]

(In reply to Isaac Aggrey [:isaac] from comment #44)
> Try results: https://tbpl.mozilla.org/?tree=Try&rev=f2f546c568ba
> 
> It seems like Linux builds are successful, but the other platforms have a
> variety of problems:
> 
> * Android: appears to use some security/nss code that relies on prarena.h
> types that are not defined without protypes.h

Hmm, it seems like for some reason the NSS build system is picking up -DNO_NSPR_10_SUPPORT.  CCing a bunch of folks who might be able to tell us how to avoid that.

> * OS X: need to add "mozilla/StandardInteger.h" to nsCocoaFeatures.h (maybe
> others?)

Sure, that sounds fine.  Do you have access to an OS X machine so that you don't have to push to try over and over again?  Also, note the error in this log (for optimized builds): https://tbpl.mozilla.org/php/getParsedLog.php?id=16032572&tree=Try

> * Windows: not sure...

The situation about windows is annoying since we don't get the compiler errors in the logs (see bug 797351).  Do you have access to a Windows machine?

> * ARM: not sure here either...something about Python.h not being found

The missing Python.h error is during configure and is benign, so you can ignore it.  These failures are the same as Android.  BTW, you can do local Android builds on  Linux if you need, see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

Comment 51

[(no longer active)]

Also, can we go ahead and land parts 4 and 5 while we're working on the rest?  I'm also very interested in landing part 2 soon too, since that will bitrot very fast.  Can you please see if you can get that part to compile stand-alone?  I think we might only need to fix the Mac build failure since that seems to be the only build failure caused by that part.

Thanks!

Comment 52

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Ehsan Akhgari [:ehsan] from comment #50)
> Hmm, it seems like for some reason the NSS build system is picking up
> -DNO_NSPR_10_SUPPORT.  CCing a bunch of folks who might be able to tell us
> how to avoid that.

Wan-Teh, do you think it is worthwhile to try to make NSS build with NO_NSPR_10_SUPPORT defined?

NSS gets built using security/build/Makefile.in. So, it seems we must undefine NO_NSPR_10_SUPPORT there.

I'm inclined to say that we should avoid inheriting any DEFINES from the Mozilla build environment when we build NSS.

Comment 53

[Mike Hommey [:glandium]]

Comment on attachment 670286 [details] [diff] [review]
Part 1: Build with NO_NSPR_10_SUPPORT

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

::: build/autoconf/no-nspr.m4
@@ +1,2 @@
> +CFLAGS="$CFLAGS -DNO_NSPR_10_SUPPORT"
> +CXXFLAGS="$CXXFLAGS -DNO_NSPR_10_SUPPORT"

I don't think it's a good idea to add that here. I think it would be better to put that in DEFINES in config/config.mk. That would also make it not be used when building nss.

Comment 54

[Isaac Aggrey [:isaac]]

(In reply to Ehsan Akhgari [:ehsan] from comment #50)
> > * OS X: need to add "mozilla/StandardInteger.h" to nsCocoaFeatures.h (maybe
> > others?)
> 
> Sure, that sounds fine.  Do you have access to an OS X machine so that you
> don't have to push to try over and over again?  

No, I don't unfortunately. And now that I think about it, since nsCocoaFeatures is OSX specific, I guess we can use stdint.h directly.

> Also, note the error in this log (for optimized builds):
> https://tbpl.mozilla.org/php/getParsedLog.php?id=16032572&tree=Try

Noted. This also appeared while building on Windows.

> > * Windows: not sure...
> 
> The situation about windows is annoying since we don't get the compiler
> errors in the logs (see bug 797351).  Do you have access to a Windows
> machine?

I didn't, but I went ahead and spent the last day or so re-partitioning my HDD to add Windows back. Compiling on Windows now...and ran into errors that seem unrelated to my patches:

c:/mozilla-central/widget/windows/nsDataObj.cpp(315) : error C2440: 'return' :
annot convert from 'nsresult' to 'HRESULT'
        This conversion requires an explicit cast (static_cast, C-style cast or
function-style cast)
c:/mozilla-central/widget/windows/nsDataObj.cpp(1114) : error C2440: 'return' :
cannot convert from 'nsresult' to 'HRESULT'
        This conversion requires an explicit cast (static_cast, C-style cast or
function-style cast)
c:/mozilla-central/widget/windows/nsDataObj.cpp(1117) : error C2440: 'return' :
cannot convert from 'nsresult' to 'HRESULT'
        This conversion requires an explicit cast (static_cast, C-style cast or
function-style cast)
2

Any idea?

> > * ARM: not sure here either...something about Python.h not being found
> 
> The missing Python.h error is during configure and is benign, so you can
> ignore it.  These failures are the same as Android.  BTW, you can do local
> Android builds on  Linux if you need, see the instructions here:
> https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

I will tackle building on Android as soon as I can.

Comment 55

[Isaac Aggrey [:isaac]]

Attachment: Part 1: Build with NO_NSPR_10_SUPPORT

Adding to DEFINES in config/config.mk and js/src/config/config.mk instead

Comment 56

[Wan-Teh Chang]

Comment on attachment 671148 [details] [diff] [review]
Part 4: Use PR_NewLogModule instead of PR_LOG_DEFINE

r=wtc. Thanks!

Comment 57

[Wan-Teh Chang]

(In reply to Brian Smith (:bsmith) from comment #52)
>
> Wan-Teh, do you think it is worthwhile to try to make NSS build with
> NO_NSPR_10_SUPPORT defined?

Yes, I think it is worthwhile.

> I'm inclined to say that we should avoid inheriting any DEFINES from the
> Mozilla build environment when we build NSS.

I agree. It makes sense to pass some compiler flags to NSS, but the
-D flags in general should be controlled by NSS itself.

Comment 58

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #57)
> (In reply to Brian Smith (:bsmith) from comment #52)
> >
> > Wan-Teh, do you think it is worthwhile to try to make NSS build with
> > NO_NSPR_10_SUPPORT defined?
> 
> Yes, I think it is worthwhile.

OK, then let's do the NSS changes in another bug, because NSS code style and review tends to be a little bit different, and I'd like these patches to land soon so I can land the patches in bug 802378 and others on top of them.

I filed NSS bug 802430 for getting NSS to build with NO_NSPR_10_SUPPORT defined.

Comment 59

[(no longer active)]

(In reply to comment #54)
> (In reply to Ehsan Akhgari [:ehsan] from comment #50)
> > > * OS X: need to add "mozilla/StandardInteger.h" to nsCocoaFeatures.h (maybe
> > > others?)
> > 
> > Sure, that sounds fine.  Do you have access to an OS X machine so that you
> > don't have to push to try over and over again?  
> 
> No, I don't unfortunately. And now that I think about it, since nsCocoaFeatures
> is OSX specific, I guess we can use stdint.h directly.

I guess so, but I would prefer StandardInteger.h for consistency.

> > > * Windows: not sure...
> > 
> > The situation about windows is annoying since we don't get the compiler
> > errors in the logs (see bug 797351).  Do you have access to a Windows
> > machine?
> 
> I didn't, but I went ahead and spent the last day or so re-partitioning my HDD
> to add Windows back. Compiling on Windows now...and ran into errors that seem
> unrelated to my patches:
> 
> c:/mozilla-central/widget/windows/nsDataObj.cpp(315) : error C2440: 'return' :
> annot convert from 'nsresult' to 'HRESULT'
>         This conversion requires an explicit cast (static_cast, C-style cast or
> function-style cast)
> c:/mozilla-central/widget/windows/nsDataObj.cpp(1114) : error C2440: 'return' :
> cannot convert from 'nsresult' to 'HRESULT'
>         This conversion requires an explicit cast (static_cast, C-style cast or
> function-style cast)
> c:/mozilla-central/widget/windows/nsDataObj.cpp(1117) : error C2440: 'return' :
> cannot convert from 'nsresult' to 'HRESULT'
>         This conversion requires an explicit cast (static_cast, C-style cast or
> function-style cast)
> 2
> 
> Any idea?

If you're using Visual C++ 2012, you're hitting bug 801471, which has been fixed on mozilla-central now.  Try rebasing your patches on top of the mozilla-central tip.

> > > * ARM: not sure here either...something about Python.h not being found
> > 
> > The missing Python.h error is during configure and is benign, so you can
> > ignore it.  These failures are the same as Android.  BTW, you can do local
> > Android builds on  Linux if you need, see the instructions here:
> > https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec
> 
> I will tackle building on Android as soon as I can.

Great, thanks!

Comment 60

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Here's a TBPL run with some (all?) of the fixups I needed to build on Windows win VS2010:
https://tbpl.mozilla.org/?tree=Try&rev=06129e1057b9

In particular, my local VS2010 build succeeded w/ this:

    https://hg.mozilla.org/try/rev/28bd48a2f385 

And hopefully trace-malloc-enabled Windows debug build, and the Mac build will succeed with this:

    https://hg.mozilla.org/try/rev/70b8df3ee754

Note that the trace-malloc patch also fixes some indention issues.

Comment 61

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Sigh. I meant:

https://tbpl.mozilla.org/?tree=Try&rev=50755cc3fc6f
https://hg.mozilla.org/try/rev/13b6bfc59046
https://hg.mozilla.org/try/rev/28bd48a2f385

Comment 62

[(no longer active)]

(In reply to comment #60)
> Here's a TBPL run with some (all?) of the fixups I needed to build on Windows
> win VS2010:
> https://tbpl.mozilla.org/?tree=Try&rev=06129e1057b9
> 
> In particular, my local VS2010 build succeeded w/ this:
> 
>     https://hg.mozilla.org/try/rev/28bd48a2f385 

This is great.  Why did you have to add the toolkit/xre/nsConsoleWriter.cpp hunk?  We should try to avoid doing that.

Please attach patches here for review once they pass try.  Thanks!

Comment 63

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 6 - Fixups needed to build on Windows and Android and Mac OS X debug, plus shorten comment to be less than 80 columns

Please feel free to fold these changes into the existing patches, if you are not going to fold all these patches together.

I will comment on the specific changes I made using the Splinter tool. I recommend using the Splinter tool to read my comment.

Comment 64

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 672163 [details] [diff] [review]
Part 6 - Fixups needed to build on Windows and Android and Mac OS X debug, plus shorten comment to be less than 80 columns

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

::: extensions/gio/nsGIOProtocolHandler.cpp
@@ +6,5 @@
>  /*
>   * This code is based on original Mozilla gnome-vfs extension. It implements
>   * input stream provided by GVFS/GIO.
>  */
> +#include "NSPRFormatTime.h" // must be before anything that includes prtime.h

Fixed line length. (~12X)

::: tools/trace-malloc/lib/nsDebugHelpWin32.cpp
@@ +223,5 @@
>  
>      // do the fun stuff...
>  
>      PIMAGE_IMPORT_DESCRIPTOR desc;
> +    ULONG size;

The existing code relied on uint32 being the same primitive type as ULONG. uint32_t is not the same type as ULONG.

::: tools/trace-malloc/lib/nsTraceMalloc.c
@@ +96,5 @@
>      char        *buf;
>      int         bufsize;
>      int         pos;
> +    uint32_t    size;
> +    uint32_t    simsize;

alignment.

@@ +267,5 @@
>      return t;
>  }
>  
>  /* We don't want more than 32 logfiles open at once, ok? */
> +typedef uint32_t        lfd_set;

alignment.

::: tools/trace-malloc/tmreader.h
@@ +25,5 @@
>  typedef struct tmmethodnode tmmethodnode;
>  
>  struct tmevent {
>      char            type;
> +    uint32_t        serial;

More alignment changes.

::: xpcom/base/NSPRFormatTime.h
@@ +5,5 @@
>  
>  // Temporarily undefine to build successfully with NO_NSPR_10_SUPPORT
> +#ifdef prtime_h___
> +#error NSPRFormatTime.h must be included before any header that includes prtime.h
> +#endif

try to detect some badness.

BTW, please file a bug for getting rid of NSPRFormatTime.

::: xpcom/glue/nsCRTGlue.cpp
@@ +251,5 @@
>  {
>    // turn PR_Now() into milliseconds since epoch
>    // and salt rand with that.
>    double fpTime = double(PR_Now());
> +  srand((unsigned int)(fpTime * 1e-6 + 0.5));   // use 1e-6, granularity of PR_Now() on the mac is seconds

Why do we duplicate this code in 3+ places?

Comment 65

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

BTW, I think that Mac OS X opt will still fail in 

nsPresArena.cpp:135:16: error: use of undeclared identifier 'caddr_t'

https://tbpl.mozilla.org/php/getParsedLog.php?id=16181497&tree=Try

Also, this changeset contains a change to NSS. I will pull that out to put it into the NSS bug.

Comment 66

[(no longer active)]

Comment on attachment 672163 [details] [diff] [review]
Part 6 - Fixups needed to build on Windows and Android and Mac OS X debug, plus shorten comment to be less than 80 columns

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

::: extensions/gio/nsGIOProtocolHandler.cpp
@@ +6,5 @@
>  /*
>   * This code is based on original Mozilla gnome-vfs extension. It implements
>   * input stream provided by GVFS/GIO.
>  */
> +#include "NSPRFormatTime.h" // must be before anything that includes prtime.h

Nice! :)

::: tools/trace-malloc/lib/nsDebugHelpWin32.cpp
@@ +223,5 @@
>  
>      // do the fun stuff...
>  
>      PIMAGE_IMPORT_DESCRIPTOR desc;
> +    ULONG size;

Fair enough.

::: xpcom/glue/nsCRTGlue.cpp
@@ +251,5 @@
>  {
>    // turn PR_Now() into milliseconds since epoch
>    // and salt rand with that.
>    double fpTime = double(PR_Now());
> +  srand((unsigned int)(fpTime * 1e-6 + 0.5));   // use 1e-6, granularity of PR_Now() on the mac is seconds

Silliness.  ;-)

Comment 67

[(no longer active)]

(In reply to Brian Smith (:bsmith) from comment #65)
> BTW, I think that Mac OS X opt will still fail in 
> 
> nsPresArena.cpp:135:16: error: use of undeclared identifier 'caddr_t'
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=16181497&tree=Try

Argh.  So, google tells me that caddr_t is an obsolete BSD typedef for char*.  On those systems, mmap for example used to take a caddr_t as its first argument.  I checked with gaston on IRC and this is no longer the case today on OpenBSD.  So you should be able to remove those caddr_t casts in the mmap calls in that file.

> Also, this changeset contains a change to NSS. I will pull that out to put
> it into the NSS bug.

Which patch do you mean?

Comment 68

[(no longer active)]

Also, note that with attachment 671733 [details] [diff] [review], we should be able to land this bug without having to wait for bug 802430.

Comment 69

[(no longer active)]

Comment on attachment 670472 [details] [diff] [review]
Part 3: Use new header NSPRFormatTime.h when temp undef of NO_NSPR_10_SUPPORT required

https://hg.mozilla.org/integration/mozilla-inbound/rev/93dc400ae049

Comment 70

[(no longer active)]

Comment on attachment 671148 [details] [diff] [review]
Part 4: Use PR_NewLogModule instead of PR_LOG_DEFINE

https://hg.mozilla.org/integration/mozilla-inbound/rev/01e26f56f078

Comment 71

[(no longer active)]

Comment on attachment 670292 [details] [diff] [review]
Part 5: Use plarena.h types

https://hg.mozilla.org/integration/mozilla-inbound/rev/396742c90b0a

Comment 72

[(no longer active)]

New try run with all of the patches:

https://tbpl.mozilla.org/?tree=Try&rev=4d9a82a585ec

Comment 73

[Wan-Teh Chang]

Comment on attachment 672163 [details] [diff] [review]
Part 6 - Fixups needed to build on Windows and Android and Mac OS X debug, plus shorten comment to be less than 80 columns

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

r=wtc.

::: xpcom/glue/nsCRTGlue.cpp
@@ +251,5 @@
>  {
>    // turn PR_Now() into milliseconds since epoch
>    // and salt rand with that.
>    double fpTime = double(PR_Now());
> +  srand((unsigned int)(fpTime * 1e-6 + 0.5));   // use 1e-6, granularity of PR_Now() on the mac is seconds

We should find out if the PR_Now() granularity is
still seconds on the Mac today. If so, we should fix
that. That is most likely due to the limitation of
Classic Mac OS.

Comment 74

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 7 - Stop using caddr_t in layout to fix Mac OS X build

Ehsan, I don't have a Mac. It is good for somebody that can easily do Mac builds to take over this part.

Also, the Android builds fail without the patch to NSS from bug 802430. That patch is very simple. However, I think it might be possible to simply remove the offending #includes from mozglue/android; if so, I will attach a patch that does that.

I pushed both of these changes to https://tbpl.mozilla.org/?tree=Try&rev=908df08aa5af

Comment 75

[(no longer active)]

(In reply to comment #74)
> Created attachment 672368 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=672368&action=edit
> Stop using caddr_t in layout to fix Mac OS X build
> 
> Ehsan, I don't have a Mac. It is good for somebody that can easily do Mac
> builds to take over this part.

I'll do that.

> Also, the Android builds fail without the patch to NSS from bug 802430. That
> patch is very simple. However, I think it might be possible to simply remove
> the offending #includes from mozglue/android; if so, I will attach a patch that
> does that.
> 
> I pushed both of these changes to
> https://tbpl.mozilla.org/?tree=Try&rev=908df08aa5af

Great!

Comment 76

[Isaac Aggrey [:isaac]]

(In reply to Brian Smith (:bsmith) from comment #64)
> Comment on attachment 672163 [details] [diff] [review]
> Fixups needed to build on Windows and Android and Mac OS X debug, plus
> shorten comment to be less than 80 columns

By the way, thanks for these fixes!

Bug 802706 filed to remove NSPRFormatTime.h. Sorry for Bugzilla spam, I thought I changed who would be CC'd since I cloned this bug and modified it for simplicity.

Comment 77

[Mozilla RelEng Bot]

Try run for 4d9a82a585ec is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4d9a82a585ec
Results (out of 16 total builds):
    success: 11
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4d9a82a585ec

Comment 78

[(no longer active)]

Attachment: Part 8 - Fix mac builds

Comment 79

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 672425 [details] [diff] [review]
Part 8 - Fix mac builds

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

::: layout/base/nsPresArena.cpp
@@ +137,5 @@
>  
>  static void
>  ReleaseRegion(void *region, uintptr_t size)
>  {
> +  munmap(reinterpret_cast<void*>(region), size);

cast is not necessary here.

Comment 80

[(no longer active)]

Attachment: Part 8 - Fix mac builds

oh, right.  Not sure why I did that!  ;-)

Comment 81

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 672443 [details] [diff] [review]
Part 8 - Fix mac builds

M meant to r+ the previous one anyway. :)

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93dc400ae049
https://hg.mozilla.org/mozilla-central/rev/01e26f56f078
https://hg.mozilla.org/mozilla-central/rev/396742c90b0a

Comment 83

[(no longer active)]

Comment on attachment 672443 [details] [diff] [review]
Part 8 - Fix mac builds

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a016c00dcc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/602330b6745c

(the patch is split across two changesets because of my mistake)

Comment 84

[(no longer active)]

Comment on attachment 672368 [details] [diff] [review]
Part 7 - Stop using caddr_t in layout to fix Mac OS X build

This is not needed any more.

Comment 85

[(no longer active)]

Seeing if we can land part 2 stand-alone...
https://tbpl.mozilla.org/?tree=Try&rev=7ec32b7c3dcd

Comment 86

[(no longer active)]

Comment on attachment 670606 [details] [diff] [review]
Part 2: Use stdint types

https://hg.mozilla.org/integration/mozilla-inbound/rev/52b59d7cff7c

Comment 87

[(no longer active)]

Comment on attachment 672163 [details] [diff] [review]
Part 6 - Fixups needed to build on Windows and Android and Mac OS X debug, plus shorten comment to be less than 80 columns

https://hg.mozilla.org/integration/mozilla-inbound/rev/62627192e156

Comment 88

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a016c00dcc3
https://hg.mozilla.org/mozilla-central/rev/602330b6745c

Comment 89

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: remove security/nss/cmd/** includes from mozglue

mozglue should not be including headers from security/nss/cmd/* anyway.

Comment 90

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=ebff923e50e0

Comment 91

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52b59d7cff7c
https://hg.mozilla.org/mozilla-central/rev/62627192e156

Comment 92

[(no longer active)]

And here are the final pieces:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a45fac177fc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/677549e570d9

Thanks everyone for your help here! :-)

Comment 93

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a45fac177fc8
https://hg.mozilla.org/mozilla-central/rev/677549e570d9