Closed Bug 472753 Opened 11 years ago Closed 11 years ago

OS/2 build breaks in nsComponentManager.cpp

Categories

(Core :: XPCOM, defect, major)

x86
OS/2
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: wuno, Assigned: mozilla)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.2a1pre) Gecko/20090103 Minefield/3.2a1pre
Build Identifier: 

After checkin of bug 462497 OS/2 build breaks
g++ -o nsComponentManager.o -c  -DMOZILLA_INTERNAL_API -DOSTYPE=\"OS22.45\" -DOSARCH=OS2 -D_IMPL_NS_COM -IE:/usr/src/hg/mozilla-central/xpcom/components/../reflect/xptinfo/src -IE:/usr/src/hg/mozilla-central/xpcom/components/../base -IE:/usr/src/hg/mozilla-central/xpcom/components/../thread -IE:/usr/src/hg/mozilla-central/xpcom/components/../ds -IE:/usr/src/hg/mozilla-central/xpcom/components/../build -I..  -IE:/usr/src/hg/mozilla-central/xpcom/components -I. -I../../dist/include/string -I../../dist/include   -I../../dist/include/xpcom -IE:/mozbuild/dist/include/nspr           -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -pedantic -fno-strict-aliasing -Zomf -pipe  -DNDEBUG -DTRIMMED -O2   -DMOZILLA_CLIENT -include ../../mozilla-config.h -Uunix -U__unix -U__unix__ -Wp,-MD,.deps/nsComponentManager.pp E:/usr/src/hg/mozilla-central/xpcom/components/nsComponentManager.cpp
In file included from E:/usr/src/hg/mozilla-central/xpcom/components/nsComponentManager.cpp:87:
E:/mozbuild/dist/include/nspr/private/pprthred.h:354: error: `
   EXCEPTIONREGISTRATIONRECORD' was not declared in this scope
E:/mozbuild/dist/include/nspr/private/pprthred.h:354: error: `e' was not 
   declared in this scope
E:/mozbuild/dist/include/nspr/private/pprthred.h:354: warning: `
   PR_OS2_SetFloatExcpHandler' initialized and declared `extern'
E:/mozbuild/dist/include/nspr/private/pprthred.h:354: error: variable or field 
   `PR_OS2_SetFloatExcpHandler' declared void
E:/mozbuild/dist/include/nspr/private/pprthred.h:354: error: variable `int 
   PR_OS2_SetFloatExcpHandler' definition is marked dllimport.
E:/mozbuild/dist/include/nspr/private/pprthred.h:355: error: `
   EXCEPTIONREGISTRATIONRECORD' was not declared in this scope
E:/mozbuild/dist/include/nspr/private/pprthred.h:355: error: `e' was not 
   declared in this scope
E:/mozbuild/dist/include/nspr/private/pprthred.h:355: warning: `
   PR_OS2_UnsetFloatExcpHandler' initialized and declared `extern'
E:/mozbuild/dist/include/nspr/private/pprthred.h:355: error: variable or field 
   `PR_OS2_UnsetFloatExcpHandler' declared void
E:/mozbuild/dist/include/nspr/private/pprthred.h:355: error: variable `int 
   PR_OS2_UnsetFloatExcpHandler' definition is marked dllimport.
E:/usr/src/hg/mozilla-central/xpcom/components/nsComponentManager.cpp: In 
   member function `void 
   nsComponentManagerImpl::LoadDeferredModules(nsTArray<DeferredModule>&)':
E:/usr/src/hg/mozilla-central/xpcom/components/nsComponentManager.cpp:3122: warning: comparison
   between signed and unsigned integer expressions
E:/usr/src/hg/mozilla-central/xpcom/components/nsComponentManager.cpp: In 
   member function `virtual nsresult 
   nsComponentManagerImpl::AutoUnregisterComponent(int, nsIFile*)':
E:/usr/src/hg/mozilla-central/xpcom/components/nsComponentManager.cpp:3164: warning: comparison
   between signed and unsigned integer expressions
make.exe[5]: *** [nsComponentManager.o] Error 1


Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Blocks: 462497
Version: unspecified → Trunk
That looks like os2.h is included already before the include in pprthred.h. And that #include needs to be prefixed with #define INCL_DOS or something like that.
--- nsprpub\pr\include\private\pprthred.h.orig
+++ nsprpub\pr\include\privatepprthred.h
@@ -47,6 +47,7 @@
 #if defined(XP_OS2)
 #define INCL_DOS
 #define INCL_DOSERRORS
+#define INCL_DOSEXCEPTIONS
 #define INCL_WIN
 #include <os2.h>
 #endif

?
(In reply to comment #2)
> --- nsprpub\pr\include\private\pprthred.h.orig
> +++ nsprpub\pr\include\privatepprthred.h
> @@ -47,6 +47,7 @@
>  #if defined(XP_OS2)
>  #define INCL_DOS
>  #define INCL_DOSERRORS
> +#define INCL_DOSEXCEPTIONS
>  #define INCL_WIN
>  #include <os2.h>
>  #endif
> 
> ?

unfortunately, that doesn't work or is not enough.

I was able to complete the build by if 0ing the inclusion of private\ppthred.h

But why does the inclusion of this header work in toolkit\xre\nsAppRunner.cpp?
64 #ifdef XP_OS2
65 #include "private/pprthred.h"
66 #endif

and would we need it at all on OS/2?
Please preprocess (by changing the g++ command line from "-c -o bla.o" to "-dD -E -o bla.E") the files in question and watch where os2.h gets included first. You can then compare nsAppRunner to nsComponentManager.
Summary: OS/2 build brakes in nsComponentManager.cpp → OS/2 build breaks in nsComponentManager.cpp
(In reply to comment #4)
> Please preprocess (by changing the g++ command line from "-c -o bla.o" to "-dD
> -E -o bla.E") the files in question and watch where os2.h gets included first.
> You can then compare nsAppRunner to nsComponentManager.
I struggled with the commandline length when I copied the lines from the log for nsAppRunner to make the suggested changes (it worked for nsComponentManager). Nevertheless, when I looked at the respective ./deps/*.pp files I saw, that in nsAppRunner.pp os2.h is included directly after pprthred.h, while in nsComponentManager.pp pprthred.h is included at the end, and os2.h was included earlier. I moved the  line #include private/pprthred.h a bit around I found that it has to be included before #include nsLocalFile.h
This unbreaks the OS/2 build by moving #include nsLocalFile.h after private/pprthred.h.
Alternatively, one could also move up private/pprthred.h to be included before nsLocalFile.h. Both variants work on OS/2, and neither would brake linux nor Windows (Mac I couldn't test).
Attachment #356314 - Flags: review?(benjamin)
Walter, does it help to add
   #define INCL_DOS
and/or
   #define INCL_DOSEXCEPTIONS
to all the other INCL_ defines in xpcom/io/nsLocalFileOS2.h?
(In reply to comment #6)
> This unbreaks the OS/2 build by moving #include nsLocalFile.h after
> private/pprthred.h.

The order of #include directives shouldn't normally matter. Headers are supposed to have #include for all dependencies, and to use '#ifdef CANONICALIZEDFILENAME' to guard against multiple inclusion.

So the patch looks like a workaround. Is it possible address the cause of the problem?
Comment on attachment 356314 [details] [diff] [review]
move nsLocalFile.h after private/ppthread.h in the list of included headers

(In reply to comment #7)
> Walter, does it help to add
>    #define INCL_DOS
> and/or
>    #define INCL_DOSEXCEPTIONS
> to all the other INCL_ defines in xpcom/io/nsLocalFileOS2.h?
Both would work (didn't try the combination). Which one is more preferable?
(In reply to comment #8)
> (In reply to comment #6)
> > This unbreaks the OS/2 build by moving #include nsLocalFile.h after
> > private/pprthred.h.
> 
> The order of #include directives shouldn't normally matter. Headers are
> supposed to have #include for all dependencies, and to use '#ifdef
> CANONICALIZEDFILENAME' to guard against multiple inclusion.
> 
> So the patch looks like a workaround. Is it possible address the cause of the
> problem?

You're right, ask for r canceled in favor for looking at a valid solution for OS/2.
Attachment #356314 - Attachment is obsolete: true
Attachment #356314 - Flags: review?(benjamin)
(In reply to comment #9)
> (In reply to comment #7)
> > Walter, does it help to add
> >    #define INCL_DOS
> > and/or
> >    #define INCL_DOSEXCEPTIONS
> > to all the other INCL_ defines in xpcom/io/nsLocalFileOS2.h?
> Both would work (didn't try the combination). Which one is more preferable?
Peter, Just to test if I understood it correctly what is defined in os2tk45\h\bsedos.h: If we go with #define INCL_DOS in nsLocalFileOS2.h, we can remove except INCL_DOSERRORS all other lines with #define INCL_DOS* from nsLocalFile.h as these are then automatically defined?
Walter, yes, looks like INCL_DOS includes all of them. Please give it a try.

In reply to comment #8)
> The order of #include directives shouldn't normally matter. Headers are
> supposed to have #include for all dependencies, and to use '#ifdef
> CANONICALIZEDFILENAME' to guard against multiple inclusion.

That is correct in theory but not the case on all programming platforms. Some toolkits need #defines before including toolkit headers and then the order does matter, because the same header might be included from different places...

> So the patch looks like a workaround. Is it possible address the cause of the
> problem?

...but yes, in this case we should be able to fix it in OS/2-specific files. Although I am really wondering if private/pprthred.h was meant to be included into stuff outside of NSPR (given the name of the path).
Attached patch fixSplinter Review
Fix as suggested by Walter in comment 10.

It so far at least got me through compiling XPCOM, if the build completes and runs I'll check it in.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Took a bit longer because it got too late yesterday, but now I pushed it (http://hg.mozilla.org/mozilla-central/rev/b945b4f67e7e).
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #13)
> Took a bit longer because it got too late yesterday, but now I pushed it
> (http://hg.mozilla.org/mozilla-central/rev/b945b4f67e7e).

Peter, could you checkin the patch also to the branch since bug462497 was made it to mozilla-1.9.1 (http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?changeset=7d740ae63d5c)
Sorry, it appears that I completely missed bugmail about this and only found about this when trying to build the next set of nightlies. Pushed to branch now:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/71fb8ce8014f
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.