GNOME Bugzilla – Bug 537328
ORBit: patches for ORBit for MSVC support
Last modified: 2009-10-17 23:01:40 UTC
Please describe the problem: This is a bigger patch, so I broke it into three separate parts. With these patches (and the already submitted ones) I'm able to build ORBit2 on Windows, and also build applications that use ORBit (and even run them :-)). Other than some of the included ORBit tests plus other tests I wrote myself, I also compiled a bunch of GNOME stuff to make sure I didn't break anything (including bonobo, and also gnumeric, so I could run something). In general, this shouldn't change anything for the current code. It will only cause differences if you use any of the new command line switches to orbit-idl-2, otherwise everything should work exactly as before. Patches coming up shortly. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 112382 [details] [review] Patch for ORBit's header files Changes to the ORBit header files to tag data exports. Windows DLL require data export to be tagged with "__declspec(dllimport)" in header files, otherwise you get errors (unresolved symbols). This is slightly different from libIDL's approach, since it also tags data exports in the header (instead of using the .def file). This is to make it easier to use the same tag for both the "hand crafted" headers and the generated code, which doesn't use .def files.
Created attachment 112383 [details] [review] Patch for ORBit's source code This cleans up some warnings of Win32, and also includes generated headers in the implementation source file, so the functions are tagged properly for export in the DLL.
Created attachment 112384 [details] [review] Patch for ORBit's IDL compiler This is the bigger patch. It adds two command line arguments to orbit-idl-2: . --dll-tag: annotates functions that need to be exported in a DLL with the given tag. The code also generates the macro definitions. . --disable-static-init: changes the way generated code initializes the type code structures (and some other stuff). Win32 compilers don't like the static initialization currently done, so move initialization to a function that is called when the DLL is loaded. To be able to compile generated code on Win32, this option must be used (unless you're compiling ORBit itself, then it's not necessary). A lot of the changes is just re-indenting existing code. The new initialization function is generated by calling the same code that generates the declarations a second time, saying that instead of declaring things it should generate initialization code.
Hmm, so this is not then after all using the __attribute__((__constructor__))) trick with gcc and the corresponding #pragma data_seg(".CRT$XCU") trick with MSVC? Wouldn't that be simpler, require much less changes to the code generation?
It is (see end of third patch), but it's not as simple as just using that. You have to do all the initialization of the variables inside a function - you can't use static initializers like the current generated code does. And that's the biggest part of the change.
In the first patch, what is the exact semantics of the ORBIT_DLL preprocessor macro? I assume it means "we are now compiling source code that goes into the ORBit DLL (and not code that just uses the ORBit DLL)". Then its name is a bit misleading. ORBIT_DLL sounds to me like it would mean in general that ORBIT is a DLL (ant not a static library). In GLib we use GLIB_COMPILATION, and in libIDL LIBIDL_COMPILATION, so the consistent name to use would be ORBIT_COMPILATION (or LIBORBIT_DLL_COMPILATION to be even more explicit). You should ideally also provide patches that add -DORBIT_COMPILATION to the Makefile.am files in the relevant sources folders. Although if you don't use the auto* build system, those files are probably greek to you. Is the #define getpid _getpid really necessary? getpid is in oldnames.lib, doesn't MSVC link with that by default? (Ah, I see, they hide its declaration in the process.h header if !__STDC__. That's a silly thing to do. Surely if one includes a header like process.h that is not a C standard header by itself already, but a totally Microsoft-specific invention, one *expects* it do declare stuff that is non-standard? Sheesh... I can understand they they want to avoid declaring non-standard functions in standard headers like stdio.h, but process.h won't be included by accident from code that intends to use only the standard C library. They have no such qualms about windows.h, for instance... and process.h is not any more cross-platform than windows.h is.)
> It is (see end of third patch) Ah sorry, it must be too early in the morning for me;)
Created attachment 112387 [details] [review] Patch for ORBit's header files (updated) ORBIT_DLL -> ORBIT_COMPILATION
Created attachment 112388 [details] [review] Patch for ORBit's IDL compiler (updated) *_DLL -> *_COMPILATION
Created attachment 112389 [details] [review] Patch for ORBit's header files (updated) Sorry for the spam, I messed up the previous patch.
Created attachment 112390 [details] [review] Patch for ORBit's header files (updated) Seriously, today is not my day. Sorry.
Created attachment 112391 [details] [review] Patch for ORBit's IDL compiler (updated) Now it all should be right.
(I swear I had sent this comment before.) I don't know what is MS's reason for renaming all functions to have a leading "_". With those changes I'm just trying to remove the nasty and annoying warnings that are printed if you call the functions with the usual name. I'm not currently using autotools to build ORBit, so I haven't made any changes in that area. I may take a look at it later - although I have just a very vague idea of how that stuff works, it may be a way to learn a little bit about it. There are also the patches sent by Michael to the ORBit list, which I haven't tried myself.
Sorry for not doing anything about this for a while. Actually now, I guess, could be a good time to get back to this issue, as we now have a separate stable branch of ORBit2 in SVN and changes like this would go into the development branch (trunk) only (but hopefully still get some testing by developers who run stuff from trunk). Then at the next GNOME release the development branch would become the stable (or only) one.
Are these patches tested and found ok to commit?
Not really. There isn't much interest in ORBit2 on Win32 from my side at least currently.
Marcelo ! doh - I'm so sorry about this - I believe that we now have working Win32 support in ORBit2 HEAD - and it is building & running ok on the platform there; so I'm going to close this out. Apologies for the slow response; can you check that there are no features missing that you need in the latest version ? Thanks.
Michael, we have support for building ORBit2 (and ORBit2-using code) with gcc. Not with te Microsoft toolchain, which is what Marcelo's patches are about. There *are* significant differences in how the two toolchains behave especially for some of the stuff that is involved, like variables imported from DLLs.
Michael, Tor already touched on why these changes are needed for MSVC support (not just Win32 support as your comment suggests). That being said, we've moved on to using a different architecture for the project where we'd be using ORBit, so I haven't looked at any of this in a while... feel free to use the patches if you'd like, but there's not much interested on my side for them anymore. (BTW RESOLVED FIXED is probably not right here.)