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

RESOLVED FIXED in mozilla19

Status

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: Ehsan, Assigned: isaac)

Tracking

(Blocks 1 bug)

Trunk
mozilla19
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=ehsan][lang=c++])

Attachments

(9 attachments, 11 obsolete attachments)

904 bytes, text/plain
Details
18.53 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
11.37 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
178.66 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
2.51 KB, patch
Ehsan
: review+
wtc
: review+
Details | Diff | Splinter Review
4.52 KB, patch
glandium
: review+
wtc
: review+
Details | Diff | Splinter Review
20.88 KB, patch
Ehsan
: review+
wtc
: review+
Details | Diff | Splinter Review
1.80 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
2.54 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Reporter

Description

7 years ago
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!
Reporter

Comment 1

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

Comment 2

7 years ago
(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!
Assignee

Updated

7 years ago
Assignee: nobody → isaac.aggrey
Assignee

Comment 3

7 years ago
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)
Reporter

Comment 4

7 years ago
(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!  :-)
Assignee

Comment 5

7 years ago
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?
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.
Reporter

Comment 7

7 years ago
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.
Reporter

Updated

7 years ago
Assignee

Comment 8

7 years ago
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?
Status: NEW → ASSIGNED
Flags: needinfo?
Reporter

Comment 9

7 years ago
(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!
Flags: needinfo?
(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.
Assignee

Comment 11

7 years ago
(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?
Reporter

Comment 12

7 years ago
(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.
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.
Reporter

Comment 14

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

Comment 15

7 years ago
(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.
Reporter

Comment 17

7 years ago
(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!
Assignee

Comment 18

7 years ago
This version avoids clobbering what seem like other upstream or third party projects in mozilla-central that caused build issues when touched.
Attachment #670175 - Attachment is obsolete: true
Assignee

Comment 19

7 years ago
Attachment #670286 - Flags: review?(ehsan)
Assignee

Comment 20

7 years ago
Posted patch Part 2: Use stdint types (obsolete) — Splinter Review
Attachment #670288 - Flags: review?(ehsan)
Assignee

Comment 21

7 years ago
I'm not sure if the comment in NSPRFormatTime.h is sufficiently descriptive.
Attachment #670290 - Flags: review?(ehsan)
Assignee

Comment 22

7 years ago
Same here on comment in NSPRLog.h
Attachment #670291 - Flags: review?(ehsan)
Assignee

Comment 23

7 years ago
Attachment #670292 - Flags: review?(ehsan)
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.
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/isaac.aggrey@gmail.com-bc65f8e1b8c9
Assignee

Comment 26

7 years ago
(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.
Assignee

Comment 27

7 years ago
Posted patch Part 2: Use stdint types (obsolete) — Splinter Review
Incorporated Ms2ger's review
Attachment #670288 - Attachment is obsolete: true
Attachment #670288 - Flags: review?(ehsan)
Attachment #670335 - Flags: review?(ehsan)
(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/
Reporter

Comment 29

7 years ago
(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.
Reporter

Comment 30

7 years ago
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.
Attachment #670286 - Flags: review?(mh+mozilla)
Attachment #670286 - Flags: review?(ehsan)
Attachment #670286 - Flags: review+
Reporter

Comment 31

7 years ago
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.
Attachment #670290 - Flags: review?(ehsan) → review-
Reporter

Comment 32

7 years ago
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.
Attachment #670291 - Flags: review?(ehsan) → review-
Reporter

Updated

7 years ago
Attachment #670292 - Flags: review?(ehsan) → review+
Assignee

Comment 34

7 years ago
Attachment #670291 - Attachment is obsolete: true
Attachment #670473 - Flags: review?(ehsan)
Assignee

Updated

7 years ago
Attachment #670472 - Attachment description: Use new header NSPRFormatTime.h when temp undef of NO_NSPR_10_SUPPORT required → Part 3: Use new header NSPRFormatTime.h when temp undef of NO_NSPR_10_SUPPORT required
Assignee

Updated

7 years ago
Attachment #670473 - Attachment description: Use new header NSPRLog.h when temp undef of NO_NSPR_10_SUPPORT required → Part 4: Use new header NSPRLog.h when temp undef of NO_NSPR_10_SUPPORT required
Assignee

Comment 35

7 years ago
Posted patch Part 2: Use stdint types (obsolete) — Splinter Review
No longer touching media/webrtc or netwerk/sctp
Attachment #670335 - Attachment is obsolete: true
Attachment #670335 - Flags: review?(ehsan)
Attachment #670478 - Flags: review?(ehsan)
Reporter

Comment 36

7 years ago
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!
Attachment #670335 - Attachment is obsolete: false
Reporter

Updated

7 years ago
Attachment #670472 - Flags: review?(ehsan) → review+
Reporter

Updated

7 years ago
Attachment #670473 - Flags: review?(ehsan) → review+
Reporter

Comment 37

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

Oh, sorry for not providing feedback on the previous version in time...
Attachment #670478 - Flags: review?(ehsan)
Assignee

Updated

7 years ago
Attachment #670335 - Attachment is obsolete: true
Assignee

Comment 38

7 years ago
Posted patch Part 2: Use stdint types (obsolete) — Splinter Review
No longer touching js/src/ion or netwerk/srtp
Attachment #670478 - Attachment is obsolete: true
Attachment #670601 - Flags: review?(ehsan)
Assignee

Updated

7 years ago
Attachment #670601 - Attachment description: Use stdint types → Part 2: Use stdint types
Assignee

Comment 39

7 years ago
Let's try that again...
Attachment #670601 - Attachment is obsolete: true
Attachment #670601 - Flags: review?(ehsan)
Attachment #670606 - Flags: review?(ehsan)
Reporter

Comment 40

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

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

Looks good!
Attachment #670606 - Flags: review?(ehsan) → review+
Reporter

Comment 41

7 years ago
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!
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/isaac.aggrey@gmail.com-e4707365d7a2
Assignee

Comment 43

7 years ago
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).
Assignee

Comment 44

7 years ago
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

7 years ago
(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.
Assignee

Comment 46

7 years ago
(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

7 years ago
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.
Assignee

Comment 48

7 years ago
(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.
Assignee

Comment 49

7 years ago
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. :-)
Attachment #670473 - Attachment is obsolete: true
Attachment #671148 - Flags: review?(ehsan)
Reporter

Updated

7 years ago
Attachment #671148 - Flags: review?(ehsan) → review+
Reporter

Comment 50

7 years ago
(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
Reporter

Comment 51

7 years ago
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!
(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 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.
Attachment #670286 - Flags: review?(mh+mozilla) → review-
Assignee

Comment 54

7 years ago
(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.
Assignee

Comment 55

7 years ago
Adding to DEFINES in config/config.mk and js/src/config/config.mk instead
Attachment #670286 - Attachment is obsolete: true
Attachment #671733 - Flags: review?(mh+mozilla)
Attachment #671733 - Flags: review?(mh+mozilla) → review+

Comment 56

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

r=wtc. Thanks!
Attachment #671148 - Flags: review+

Comment 57

7 years ago
(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.
(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.
Reporter

Comment 59

7 years ago
(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!
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.
Reporter

Comment 62

7 years ago
(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!
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.
Attachment #672163 - Flags: review?(ehsan)
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?
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.
Reporter

Comment 66

7 years ago
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.  ;-)
Attachment #672163 - Flags: review?(ehsan) → review+
Reporter

Comment 67

7 years ago
(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?
Reporter

Comment 68

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

Comment 69

7 years ago
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
Attachment #670472 - Flags: checkin+
Reporter

Comment 70

7 years ago
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
Attachment #671148 - Flags: checkin+
Reporter

Updated

7 years ago
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++][leave open]
Reporter

Comment 72

7 years ago
New try run with all of the patches:

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

Comment 73

7 years ago
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.
Attachment #672163 - Flags: review+

Updated

7 years ago
Attachment #671733 - Flags: review+
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
Attachment #672368 - Flags: review?(ehsan)
Reporter

Comment 75

7 years ago
(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!
Reporter

Updated

7 years ago
Attachment #672368 - Attachment description: Stop using caddr_t in layout to fix Mac OS X build → Part 7 - Stop using caddr_t in layout to fix Mac OS X build
Attachment #672368 - Flags: review?(ehsan) → review+
Assignee

Updated

7 years ago
Blocks: 802706
Reporter

Updated

7 years ago
Attachment #672163 - Attachment description: Fixups needed to build on Windows and Android and Mac OS X debug, plus shorten comment to be less than 80 columns → Part 6 - Fixups needed to build on Windows and Android and Mac OS X debug, plus shorten comment to be less than 80 columns
Assignee

Comment 76

7 years ago
(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.
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/eakhgari@mozilla.com-4d9a82a585ec
Reporter

Comment 78

7 years ago
Posted patch Part 8 - Fix mac builds (obsolete) — Splinter Review
Attachment #672425 - Flags: review?(bsmith)
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.
Attachment #672425 - Flags: review?(bsmith) → review-
Reporter

Comment 80

7 years ago
oh, right.  Not sure why I did that!  ;-)
Attachment #672425 - Attachment is obsolete: true
Attachment #672443 - Flags: review?(bsmith)
Comment on attachment 672443 [details] [diff] [review]
Part 8 - Fix mac builds

M meant to r+ the previous one anyway. :)
Attachment #672443 - Flags: review?(bsmith) → review+
Reporter

Comment 83

7 years ago
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)
Attachment #672443 - Flags: checkin+
Reporter

Comment 84

7 years ago
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.
Attachment #672368 - Attachment is obsolete: true
Reporter

Comment 85

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

Comment 87

7 years ago
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
Attachment #672163 - Flags: checkin+
mozglue should not be including headers from security/nss/cmd/* anyway.
Attachment #673111 - Flags: review?(ehsan)
Attachment #673111 - Flags: review?(ehsan) → review+
Reporter

Comment 92

7 years ago
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! :-)
Whiteboard: [mentor=ehsan][lang=c++][leave open] → [mentor=ehsan][lang=c++]
https://hg.mozilla.org/mozilla-central/rev/a45fac177fc8
https://hg.mozilla.org/mozilla-central/rev/677549e570d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Duplicate of this bug: 714247
Blocks: 807938

Updated

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