Use __declspec(dllexport/dllimport) on OS/2

RESOLVED FIXED

Status

defect
RESOLVED FIXED
14 years ago
Last year

People

(Reporter: bird-mozilla, Unassigned)

Tracking

Trunk
x86
OS/2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 10 obsolete attachments)

57.47 KB, patch
mkaply
: review+
Details | Diff | Splinter Review
3.51 KB, patch
Details | Diff | Splinter Review
1.74 KB, patch
Details | Diff | Splinter Review
Reporter

Description

14 years ago
User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.7) Gecko/20040518
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.7) Gecko/20040518

As discussed in bug #281203, it's desirable to use __declspec(dllexport) for
exporting functions, classes and variables just like the Windows platform does
it. This is now available in the compiler/toolchain.

Reproducible: Always

Steps to Reproduce:




H i s t o r y
-------------

Until now the OS/2 builts has always exported every public symbol in a DLL if
the exports for that DLL was wasn't exactly defined. This means that way too
much stuff is being exported, causing more slower program loading, more kernel
memory usage, etc. At one point OS/2 started using ordinals instead of named
exports to speed up loading (giving a ~30% startup gain IIRC), however this
caused builds to be incompatible with each other because the way the ordinals
was assigned.

In bug #281203 we ran into several problems related to the sheer amount of
exports. The first problem is that OS/2 only support up to 65535 exports, making
exporting 80000 symbols impossible. The second problem is that linker (ilink v5)
have a bug which corrupts the export table if it gets to big. Tis has been
observed in dlls with 12-13000 exports a few times. Getting this bug fixed is
difficult since I'm not mainting that part of the toolchain. (A possible
workaround is to use the linker from Visual Age for C++ v3.08.)

The conclusion was that I ported the __declspec(dllexport) and
__declspec(dllimport) functionality from the mingw/cygwin GCC to the OS/2 one.
This has been done - a few bugs dealing with friends and forward declarations
had to be hacked (God knows why it works with mingw/cygwin) - and now it appears
be building seamonkey just fine. The feature is available starting with the GCC
3.3.5 / LIBC 0.6 rc1 build.


C o d e  C h a n g e s
----------------------

The changes needed to be done in a way which doesn't break the current GCC 3.2.2
builds. So, the patch is testing if __declspec is defined in order to establish
whether the compiler implements it or not - it's a built in define similar to 
#define __declspec(a) __attribute__((a)). 

The buildsystem required a little adjustment in that import libraries are
generated from the .DLL and not the .DEF-file. The FILTER operation can be
skipped too. Thus, we detect compiler support for __declspec() in the root
configure script and define MOZ_OS2_USE_DECLSPEC so that rules.mk can select the
right path.

The required changes to mozilla sources are small and largly concerned with
defining various export/import macros in the same way as done on Windows. The
proposed patch will not take the windows path but creates a separate OS/2 path -
sort of  #if win32 [stuff] #elif os2 [stuff] #else/#elif [others] - as to not
confuse/bother any windows maintainers with having to think about OS/2.

Serveral OS/2 specific classes and functions required exporting this was no big
surprise. However in one case a common class, nsBaseWidget, was also required to
be exported on OS/2. I didn't look into the details why this isn't required on
Win32.

In the case of the xptcstubs_gcc_x86_unix.cpp assembly stubs it was necessary to
add export directives.

On a couple of occations OS/2 is join Windows in using some dummy dep files for
pulling object from static libraries into DLLs.

The patch also includes two changes to OS/2 specific part which are not related
to __declspec() but non the less require for building seamonkey. These were a
(PFN *) cast in xpcom/glue/standalone/nsGlueLinkingOS2.cpp, and installing the
OS/2 widget import libary into $(DIST)/lib/components in order to make the
viewer testcase happy. The former makes sense, if it works with 3.2.2 it's a bug
in the compiler, while the latter doesn't make much sense to me (unless it's
supposed to be busted).
Reporter

Comment 1

14 years ago
Posted patch __declspec for seamonkey (obsolete) — Splinter Review
Actually I think we should take the Windows path wherever we can - it would be a
lot less work...


Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 3

14 years ago
Ok, I'll take a look at that later tonight if I get a moment.

The new GCC build is available at
ftp://ftp.netlabs.org/incoming/GCC-3.3.5-rc1.zip and will later be moved to
ftp://ftp.netlabs.org/pub/gcc/GCC-3.3.5-rc1.zip I expect.
Knuts patch works except for
+# bird: somehow this doesn't work automatically and the viewer.exe test 
+#	 depends on this being installed.
+# libs:: $(IMPORT_LIBRARY)
+#	$(INSTALL) $(IFLAGS2) $(IMPORT_LIBRARY) $(DIST)/lib/components

This fails on my static build of Seamonkey (this is with tests disabled but
Knut seemed to think it was the static part that was important).
I am attaching patch that was necessary for xulrunner build.

I also built with these changes with --disable-libxul.

Comment 5

14 years ago
Comment on attachment 190751 [details] [diff] [review]
xulrunner changes required (either due to changes in xulrunner or due to declspec -- not really sure which but as declspec is required for OS/2 to cleanly build with libxul it is needed )

This cannot possibly be correct... if MOZ_ENABLE_LIBXUL there should not *be*
an xpcom_core.dll
Attachment #190751 - Flags: review-
There is no xpcom_core.dll but xpcom_core.lib is built and I had to specify it
here, possibly because we don't have the rpath-link stuff.

Comment 7

14 years ago
You must not link against xpcom_core.lib... it's vastly wrong. Are you sure that
MOZ_COMPONENT_LIBS is set up correctly?
Posted file error without xpcom_core (obsolete) —
I get 20 unresolved externals in xpcom\stub if I don't have it as seen in
attachment.  The only lib file I found these in was xpcom_core.  For
xulrunner\app I could user xpcomglue if that is ok (needs
_GRE_GetGREPathForVersion).  I don't see a way around it for building xpcom
though.

Comment 9

14 years ago
xpcom/core should be linking with xul.lib for those symbols (import library for
xul.dll)
xul.lib does not have these symbols.  After being built xpcom.lib does but of
course that doesn't help building it.  Does this mean xul.lib is not being built
correctly?  I wouldn't expect xpcom and xul to both contain these as I would
expect a conflict.  

Comment 11

14 years ago
NS_InitXPCOM2_P should be in xul.lib/xul.dll... if it's not, we have a problem
with exports somewhere.

NS_InitXPCOM should be in xpcom.lib/xpcom.dll... if it's the _P version, that's
bad also.
That part looks ok:
[e:\cvs\work\mozilla\xulobj\dist\lib]grep NS_InitXPCOM2_P *
Binary file xpcomcomponents_s.lib matches
Binary file xpcom_core.lib matches
Binary file xul.lib matches

[e:\cvs\work\mozilla\xulobj\dist\lib]grep NS_InitXPCOM *
Binary file embed_base_s.lib matches
Binary file xpcom.lib matches
Binary file xpcomcomponents_s.lib matches
Binary file xpcomglue.lib matches
Binary file xpcom_core.lib matches
Binary file xul.lib matches
Binary file xulapp_s.lib matches

But _NS_StringGetData_P for instance:
[e:\cvs\work\mozilla\xulobj\dist\lib]grep _NS_StringGetData_P *
E:\moztools\grep.exe: components: Is a directory
Binary file xpcom.lib matches
Binary file xpcom_core.lib matches
Reporter

Comment 13

14 years ago
Comment to #c4: It should be:
ifnq ($(IMPORT_LIBRARY),,)
# bird: somehow this doesn't work automatically and the viewer.exe test 
#	depends on this being installed.
libs:: $(IMPORT_LIBRARY)
	$(INSTALL) $(IFLAGS2) $(IMPORT_LIBRARY) $(DIST)/lib/components
endif

However, it still remains to figure out why the heck OS/2 requires this to be
exported while the other targets doesn't.

Two pices of advice for the xul stuff:
1. grep will also hit unresolved externals for static libs. The best way of
making sure a symbol is really defined in a library is to use listomf and pipe
it to a decent pager (like the 4os2 list command).
2. Make sure to use include any dependency files which the Win32 guys are using.
OK, I found the secret in toolkit/library/makefile.in:
 72 ifeq ($(OS_ARCH)_$(GNU_CC),WINNT_)
 73 REQUIRES += libreg widget gfx
 74 
 75 CPPSRCS += \
 76         dlldeps.cpp \
 77         dlldeps-obs.cpp \
 78         nsGFXDeps.cpp \
 79         nsDllMain.cpp \
 80         dlldeps-xul.cpp \
 81         $(NULL)
 82 
 83 ifndef MOZ_NATIVE_ZLIB
 84 CPPSRCS += dlldeps-zlib.cpp
 85 DEFINES += -DZLIB_INTERNAL
 86 endif
 87 
 88 LOCAL_INCLUDES += -I$(topsrcdir)/widget/src/windows
 89 endif

I am not sure how to do this properly but I changed
ifeq ($(OS_ARCH)_$(GNU_CC),WINNT_)
to 
ifeq ($(OS_ARCH),OS2)
and removed 79         nsDllMain.cpp \
and line 88 changed windows to os2 (not required as it is only needed for
nsDllMain.cpp)  I then build without xpcom_core.
Attachment #190751 - Attachment is obsolete: true
Posted patch New patch (obsolete) — Splinter Review
Attachment #190762 - Attachment is obsolete: true
I have been trying to track down a problem with these declspec changes that
break the Java plugin.  http://www.javatester.org/version.html  shows "Browser
has Java disabled".  ipluginw is working as other plugins that use it work.  If
I drop back to a previous build of Mozilla without the declspec changes Java
works (using the same plugin directory, profile).  The one thing that stands out
right away is that the Java related files used _declspec rather than __declspec
where all other instances in Moz code used __declspec.  Don't know enough about
the use of _declspec/__declspec to know if this is an issue.
Further investigation into the java issue shows it is not related to the
declspec changes (just coincedence, timing-wise).  Opened Bug 303241 for java issue.

Comment 18

14 years ago
Knut, did you ever try to do the adjustment that Mike suggested in comment 2 or should I have a go at this while I try to get the patch to work in my tree? (It doesn't apply cleanly any more, anyway.)

I would also be interested to see some measure the performance gain due to these changes (like SeaMonkey or Firefox startup time). Well, when I got this running I can do that part.

Comment 19

14 years ago
Hmm, I just installed the newest GCC that says "gcc version 3.3.5 (Bird Build 2005-11-17 03:47)" but I get
   checking for __declspec(dllexport)... no
during configure, although that part of the patch applied correctly. Is there something I missed?
Posted patch correction for bitrot (obsolete) — Splinter Review
Attachment #191030 - Attachment is obsolete: true
I have been missing that the declspec was failing in configure:
Set MOZ_OS2_USE_DECLSPEC = in config/autoconf.mk to 1 and it seems to work then so I don't understand the failure.

configure:6722: checking for __declspec(dllexport)
configure:6734: g++ -c  -Zomf  -I/usr/X11R6/include  -I/usr/X11R6/include conftest.C 1>&5
configure: In function `int main()':
configure:6730: error: parse error before `{' token
configure: failed program was:
#line 6727 "configure"
#include "confdefs.h"

int main() {
__declspec(dllexport) void ac_os2_declspec(void) {}
; return 0; }


Reporter

Comment 22

14 years ago
That test is obviously wrong. Don't know why I thought it worked here. I'll get back to this when I get time again.
Posted patch updated declspec path (obsolete) — Splinter Review
Updated patch, minus the configure.in changes as the test for declspec on OS/2 has yet to be created.
Attachment #189579 - Attachment is obsolete: true
Attachment #207924 - Attachment is obsolete: true
Attachment #210506 - Flags: review?(mozilla)
Posted patch configure.in changes (obsolete) — Splinter Review
This diff is just so anyone wanting to build can do so but 
-
+        MOZ_OS2_USE_DECLSPEC='1'
+  
needs to be changed out to a test for declspec (in case 3.2.2 is in use rather than 3.3.5).

Comment 25

14 years ago
A configure.in addition that does work with both GCC 3.2.2 and 3.3.5 (setting MOZ_OS2_USE_DECLSPEC to 1 only for 3.3.5) is this:

  AC_CACHE_CHECK(for __declspec(dllexport),
     ac_os2_declspec,
     [AC_TRY_COMPILE([__declspec(dllexport) void ac_os2_declspec(void) {}],
                     [return 0;],
                     ac_os2_declspec="yes",
                     ac_os2_declspec="no")])
  if test "$ac_os2_declspec" = "yes"; then
      FILTER='true'
      MOZ_OS2_USE_DECLSPEC='1'
  fi

Unfortunately, my tree is so full of other stuff that I cannot create a proper full patch at the moment.
This patch includes Peter's configure.in update as well as some updates I had missed in xpcom in my last patch.
Attachment #210506 - Attachment is obsolete: true
Attachment #210507 - Attachment is obsolete: true
Attachment #210506 - Flags: review?(mozilla)
Attachment #214039 - Flags: review?(mozilla)
Comment on attachment 214039 [details] [diff] [review]
declspec patch

r=mkaply but I'm not sure on the jni/jri changes since those are headers from sun...
Attachment #214039 - Flags: review?(mozilla) → review+

Comment 28

14 years ago
Mike, I don't think patching those two files is a problem. The bonsai CVS logs shows several alterations to them that were done within the Mozilla codebase. They don't appear to get synched from the outside like the cairo or sqlite files are.
Posted patch nspr only changes (obsolete) — Splinter Review
NSPR only changes for seperate review.
JS and NSS patches to follow.
Posted patch NSS only changes (obsolete) — Splinter Review
Comment on attachment 214720 [details] [diff] [review]
nspr only changes

Actual relevant part of diff

Index: pr/include/prtypes.h
===================================================================
RCS file: /cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v
retrieving revision 3.20.4.13
diff -u -1 -0 -r3.20.4.13 prtypes.h
--- pr/include/prtypes.h	10 Mar 2006 05:30:56 -0000	3.20.4.13
+++ pr/include/prtypes.h	10 Mar 2006 21:11:32 -0000
@@ -89,20 +89,36 @@
 
 #define PR_EXTERN(__type) extern __declspec(dllexport) __type
 #define PR_IMPLEMENT(__type) __declspec(dllexport) __type
 #define PR_EXTERN_DATA(__type) extern __declspec(dllexport) __type
 #define PR_IMPLEMENT_DATA(__type) __declspec(dllexport) __type
 
 #define PR_CALLBACK
 #define PR_CALLBACK_DECL
 #define PR_STATIC_CALLBACK(__x) static __x
 
+#elif defined(XP_OS2) && defined(__declspec)
+
+#define PR_EXPORT(__type) extern __declspec(dllexport) __type
+#define PR_EXPORT_DATA(__type) extern __declspec(dllexport) __type
+#define PR_IMPORT(__type) extern  __declspec(dllimport) __type
+#define PR_IMPORT_DATA(__type) extern __declspec(dllimport) __type
+
+#define PR_EXTERN(__type) extern __declspec(dllexport) __type
+#define PR_IMPLEMENT(__type) __declspec(dllexport) __type
+#define PR_EXTERN_DATA(__type) extern __declspec(dllexport) __type
+#define PR_IMPLEMENT_DATA(__type) __declspec(dllexport) __type
+
+#define PR_CALLBACK
+#define PR_CALLBACK_DECL
+#define PR_STATIC_CALLBACK(__x) static __x
+
 #elif defined(XP_BEOS)
 
 #define PR_EXPORT(__type) extern __declspec(dllexport) __type
 #define PR_EXPORT_DATA(__type) extern __declspec(dllexport) __type
 #define PR_IMPORT(__type) extern __declspec(dllexport) __type
 #define PR_IMPORT_DATA(__type) extern __declspec(dllexport) __type
 
 #define PR_EXTERN(__type) extern __declspec(dllexport) __type
 #define PR_IMPLEMENT(__type) __declspec(dllexport) __type
 #define PR_EXTERN_DATA(__type) extern __declspec(dllexport) __type
Attachment #214720 - Flags: review?(wtchang)
Comment on attachment 214721 [details] [diff] [review]
JS only changes [checked into trunk]

Need someone to check this in to JS...
Attachment #214721 - Flags: review?(brendan)

Updated

14 years ago
Attachment #214722 - Flags: review?(julien.pierre.bugs)
Comment on attachment 214721 [details] [diff] [review]
JS only changes [checked into trunk]

Bouncing to mrbkap.

/be
Attachment #214721 - Flags: review?(brendan) → review?(mrbkap)
Comment on attachment 214721 [details] [diff] [review]
JS only changes [checked into trunk]

Sure, I guess. When do you want this checked in?
Attachment #214721 - Flags: review?(mrbkap) → review+

Comment 37

14 years ago
Comment on attachment 214720 [details] [diff] [review]
nspr only changes

I assume that the changes to mozilla/nsprpub/configure in this patch
should be ignored.

Do you think it's better to test the __declspec macro than to test
the GCC version?

In prtypes.h, I suggest that you put the new code right above the
(obsolete) XP_OS2_VACPP section.

Right now (before this patch is applied) OS2 GCC/EMX build uses the
default section (marked with the comment "Unix"), which uses
__attribute__((visibility("default"))) in GCC 3.3 or later.  Your
patch will lose this feature for OS2 GCC/EMX build.  If this is OK,
I will give this patch review+.
Attachment #214720 - Flags: review?(mozilla)

Comment 38

14 years ago
Comment on attachment 214722 [details] [diff] [review]
NSS only changes

The reason I marked the NSS patch obsolete is because it changes
only third-party or dead code.

zlib is third-party code.  The copy of zlib in NSS is only used
as a static library (that is, ZLIB_DLL is not defined), so NSS
doesn't need this change now.  Please remember to submit the zlib
patch to the zlib maintainers.

The entire nss/lib/fortcrypt directory is dead code.  It has
been cvs removed in NSS 3.11 (MOZILLA_1_8_0_BRANCH) or later.
So it's not necessary to improve it.
Attachment #214722 - Attachment is obsolete: true
Attachment #214722 - Flags: review?(julien.pierre.bugs)

Updated

14 years ago
Attachment #214722 - Flags: review-

Comment 39

14 years ago
I made a mistake in my previous comment.  MOZILLA_1_8_0_BRANCH
should have been MOZILLA_1_8_BRANCH.  The NSS version on the
MOZILLA_1_8_0_BRANCH is NSS 3.10.2 and still has the
nss/lib/fortcrypt directory, but that directory is dead code
nonetheless.
Sorry, I generally delete my configures before doing a diff but as I was doing a specific directory tree didn't think to do it and forgot there was a configure in that directory tree.
We don't have __attribute__((visibility("default"))) in our GCC build (we added the __declspec instead I think).  I don't know if __attribute__((visibility("default"))) is planned for a future build or not.  At this time at least we are not losing anything.
If you want the changes in prtypes.h moved I can go ahead and do that, I put them where I did as I was making them from the Windows code right above it.  
I used the __declspec rather than the version of GCC was because when I asked the maintainer of our GCC port how the best way to check if the correct version of GCC was in use he suggested the __declspec.  

Comment 41

14 years ago
Comment on attachment 214720 [details] [diff] [review]
nspr only changes

r=wtc.  I can move the code when I check in the patch.
Attachment #214720 - Flags: review?(wtchang) → review+
Comment on attachment 214721 [details] [diff] [review]
JS only changes [checked into trunk]

I checked this patch into the trunk.
Attachment #214721 - Attachment description: JS only changes → JS only changes [checked into trunk]
Attachment #214721 - Attachment is obsolete: true
Comment on attachment 214720 [details] [diff] [review]
nspr only changes

r=mkaply with your changes, wtc.

Thanks!
Attachment #214720 - Flags: review?(mozilla) → review+
wtc: I checked the patch in on the NSPR client branch, but I don't have authority to put it on the head.

Comment 45

13 years ago
mkaply: I checked the patch in on the NSPR trunk.
Attachment #214720 - Attachment is obsolete: true
Is everything checked in now?

Comment 47

13 years ago
(In reply to comment #46)
> Is everything checked in now?
> 

As far as I can see, from the xptcall changes (see comment #35) only the first part, but not the changes to xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp were checked in, the other checkins seem to be complete.

Comment 48

13 years ago
Are the __declspec changes the reason that I get about a thousand warnings like

X:\trunk\sm_debug\xpcom\ds\xpcomds_s.lib(X:\trunk\mozilla\xpcom\ds\nsAtomTable.cpp)
   : warning LNK4024: __ZN5nsCRT7IsAsciiEPKc
   : multiple definitions for export name
X:\trunk\sm_debug\xpcom\ds\xpcomds_s.lib(X:\trunk\mozilla\xpcom\ds\nsCheapSets.cpp)
   : warning LNK4024: __ZN15nsCheapInt32Set6SetIntEi
   : multiple definitions for export name

in xpcom, gfx, widget, and mailnews when building trunk with GCC 3.3.5?
I only recently recall seeing those... I may just not have been noticing them but I have been using declspec for a long time now and don't recall seeing them early on.

Comment 50

12 years ago
Hmm, comment 47 says that the checkin for xptcstubs_gcc_x86_unix.cpp was still missing, referring to attachment 214724 [details] [diff] [review]. Is that still needed? I haven't seen any build breaks for any app in the last months, at least not because of this.

I wonder why that wasn't checked in, especially as the xptcinvoke_gcc_x86_unix.cpp part of the same patch did go in. I don't understand this assembly stuff, so I cannot judge this at all. comment 0 says

   In the case of the xptcstubs_gcc_x86_unix.cpp assembly stubs it was
   necessary to add export directives.

Now we can build without that patch it's no longer necessary? Mike, Knut, Andy, Walter, any comments? Should this bug just be marked FIXED?

Comment 51

12 years ago
(Replying to comment #48) I recently did a trunk build with GCC 3.2.2 because I also was seeing those multiple symbol warnings and wondered if it was due to using __declspec. 3.2.2 gave the same warnings so it is something else.

Comment 52

12 years ago
(In reply to comment #50)
> Hmm, comment 47 says that the checkin for xptcstubs_gcc_x86_unix.cpp was still
> missing, referring to attachment 214724 [details] [diff] [review]. Is that still needed? I haven't seen

> 
> Now we can build without that patch it's no longer necessary? Mike, Knut, Andy,
> Walter, any comments? Should this bug just be marked FIXED?
> 
I add the "missing" part of it from time to time to my tree before I build. To be honest, I do not encounter more or less stability or any other obvious differences if I add it or not.
I guess, Mike could know why attachment 214724 [details] [diff] [review] was checked in only partially.

Comment 53

12 years ago
Peter, you can close this bug. Eventually, I found the solution, why the second part of attachment 214724 [details] [diff] [review] still applies and the whole thing is not broken. http://mxr.mozilla.org/mozilla/source/xpcom/reflect/xptcall/src/md/os2/xptcstubs_gcc_x86_os2.cpp This file contains the export directives. The respective Makefile calls ../xptcinvoke_gcc_x86_unix.cpp Thus the first part was checked in as in attachment 214724 [details] [diff] [review] but for the second part not an alternative solution was found(and a little bit hidden, as even in the logs hard to find).

Comment 54

12 years ago
OK, thanks for the detective work. Marking fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Product: Mozilla Application Suite → Core
Resolution: --- → FIXED
Version: unspecified → Trunk

Updated

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