Closed Bug 301064 Opened 20 years ago Closed 18 years ago

Use __declspec(dllexport/dllimport) on OS/2

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
OS/2
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bird-mozilla, Unassigned)

Details

Attachments

(3 files, 10 obsolete files)

declspec patch
57.47 KB, patch
mkaply
: review+
Details | Diff | Splinter Review
Just the xptcall changes
3.51 KB, patch
Details | Diff | Splinter Review
nspr only changes (as checked in)
1.74 KB, patch
Details | Diff | Splinter Review
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).
Attached 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
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 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.
You must not link against xpcom_core.lib... it's vastly wrong. Are you sure that MOZ_COMPONENT_LIBS is set up correctly?
Attached 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.
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.
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
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
Attached 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.
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.
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?
Attached 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; }
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.
Attached 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)
Attached 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).
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.
Attached patch declspec patch — — Splinter Review
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+
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.
Attached patch nspr only changes (obsolete) — — Splinter Review
NSPR only changes for seperate review. JS and NSS patches to follow.
Attached 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)
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 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 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)
Attachment #214722 - Flags: review-
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 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.
mkaply: I checked the patch in on the NSPR trunk.
Attachment #214720 - Attachment is obsolete: true
Is everything checked in now?
(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.
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.
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?
(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.
(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.
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).
OK, thanks for the detective work. Marking fixed.
Status: NEW → RESOLVED
Closed: 18 years ago
Product: Mozilla Application Suite → Core
Resolution: --- → FIXED
Version: unspecified → Trunk
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: