[Expat-bugs] [ expat-Patches-820946 ] expat.h not great for MSVC !cdecl deflt calling cnvntn

[SourceForge.net]

Patches item #820946, was opened at 2003-10-09 19:06
Message generated for change (Comment added) made by fdrake
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=310127&aid=820946&group_id=10127

Category: None
Group: None
Status: Open
Resolution: None
Priority: 6
Submitted By: Nobody/Anonymous (nobody)
Assigned to: Nobody/Anonymous (nobody)
Summary: expat.h not great for MSVC !cdecl deflt calling cnvntn

Initial Comment:
OK, well the title I wanted was "expat.h doesn't work
cleanly for MSVC with non-cdecl default calling
convetion", but there were (reasonable, I guess) length
limitations.

In any case, if you have a project in MSVC that has its
default calling convention set to something other than
cdecl (such as stdcall or fastcall), the prototypes for
the handler functions don't communicate that the
library is compiled to assume cdecl. Things will still
compile fine, however odd behavior occurs at runtime
(for example, my start element handler was called 5
times with the same name and then the thread crashed).
If your callback funcitons are then explicitly declared
cdecl (as they should be) then you have to use casts to
allow you to assign the address of your cdecl handler
functions to the function pointers that the compiler
defaults to the different convention.

I don't suppose many people bother to change the
default calling convention of their projects, but I
have seen fastcall give a 5% boost. XML parsing is not
a significant part of our apps exection time, so it's
not worth recompiling expat to fastcall. It would be
nice if it worked out of the box for us.

I've supplied a patch, that defines a macro XMLCALLBACK
for callbacks, much like the XMLPARSEAPI macro is
defined for API functions. I've tested it with MSVC 7.1
projects that default to both stdcall and fastcall.
Because the preprocessor tests are set up like the ones
for the XMLPARSEAPI macro, I don't think it should
affect any non-MSVC compilers.

Thanks,

Marsh Ray
marsh *period* ray *atsign* barrsystems *period* com


----------------------------------------------------------------------

>Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2003-10-13 17:47

Message:
Logged In: YES 
user_id=3066

>From looking through the CVS logs, we did have an
XMLCALLBACK macro at one point, used to mark the callback
function typedefs with cdecl.  This comment from xmlparse.c
revision 1.21 is interesting:

----
revision 1.21
date: 2001/07/27 17:17:44;  author: fdrake;  state: Exp; 
lines: +0 -4

Remove all traces of the XMLCALLBACK macro -- there appears
to be no way to add __cdecl to a typedef of a function type
that MSVC does not complain about.  Callback implementations
may need to add explicit __cdecl annotations in sources not
compiled with the C calling convention as the default.
----

XMLCALLBACK had been added (by me) in revision 1.19, which
has this comment:

----
Make sure that XMLPARSEAPI specifies the calling convention
when building under MSVC -- this is needed when using the
pre-compiled DLL with projects built using a different
calling convention.

XMLPARSEAPI now takes the return type as a parameter and
inserts annotations on both sides of the type to make sure
the compiler is happy.  A new macro, XMLCALLBACK, is used to
perform similar annotation of the callback function types,
which do not need the dllimport/dllexport annotations but do
still need the __cdecl annotation.

This closes SF bug #413653.
----

Sigh.

----------------------------------------------------------------------

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2003-10-13 17:29

Message:
Logged In: YES 
user_id=3066

Karl, the #undef XMLPARSEAPI is a mystery.  I wouldn't be
surprised if it's related to the DLL import/export stuff;
that may not be needed/desired in the implementation.  I
suspect that none of it is actually needed in the
implementation when cdecl is the default, because explicitly
stating cdecl and implicitly using it are probably
considered equivalent (questionable practice).  I'll bet we
can get away with always declaring cdecl; this impacts
expat.h as well, since there's a case when the calling
convention isn't specified there.

I suspect most of the callbacks are not performance
sensitive; given that for start/end element we already do a
fair bit of work and make many internal calls, there's
little impact based on the calling convention for the
callback.  The only one I'd suspect is particularly
sensitive is the characters callback, and there's no
specific reason to think the calling convention is going to
buy much.  I'm inclined to think that everything that
crosses the library boundary should be explicitly cdecl.

As far as XMLMEMAPI goes, I've no idea how to say "the same
calling convention as malloc()".  We could define wrapper
functions with some explicit convention, but that adds a
layer of calls; that's painful.  If you know how to pull
this one off, I'm open to suggestions!

----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2003-10-13 16:50

Message:
Logged In: YES 
user_id=290026

OK, here is what I think:

It is OK if you want to compiler with a different default
calling convention. The fact that this is a problem
with Expat is a bug IMO. We simply should fix xmlparse.c
so that those declarations that are frozen in expat.h
are also frozen in xmlparse.c.

We should look at it as two sets of calling conventions:
a) those used internally - they are basically frozen
b) those used in the API

For the second ones, we again have two groups: 
performance sensitive and non-sensitive.
The callbacks fall into the performance sensitive group,
the rest is not really performance sensitive (please correct
me if I am missing some). I think we can freeze the
non-performance sensitive API without much problems,
but we should leave the callbacks open for optimization.

So, if we concentrate on the callbacks, then it should work
if we make xmlparse.c follow expat.h.

I just patched xmlparse.c to use XMLPARSEAPI for all
functions that have it in expat.h. Down to three errors
when using another default calling convention.
The errors now are involving the memory handler callbacks.
We should probably define an XMLMEMAPI macro to freeze 
these as they should follow the calling convention used for
the standard memory allocation functions.

Btw, why is XMLPARSEAPI undefined in xmlparse.c after
inclusion of expat.h? I had to uncomment this to make
it compile. Any ideas, Fred. Marsh?

Karl




----------------------------------------------------------------------

Comment By: Fred L. Drake, Jr. (fdrake)
Date: 2003-10-13 15:53

Message:
Logged In: YES 
user_id=3066

Ok, I think I understand this one now.  ;-)  Thanks for the
careful response.

While I think that interaction between libraries and
compiler switches will probably always be a source of
frustration, we should be able to make this particular
problem go away.  I'll gladly support a solution which
always forces both call directions to always use cdecl.

Hopefully I'll be able to fix this in the next couple of
days; I know I won't have time tonight, though.  ;-(

----------------------------------------------------------------------

Comment By: Marsh Ray (marshray_bs)
Date: 2003-10-13 15:43

Message:
Logged In: YES 
user_id=883966

> How does it work when you build the library
> with your calling convention?

Expat.dll does not compile with not compile with a default
calling convention other than __cdecl. It produces 65
errors, here's a sample:

--------------------Configuration: expat - Win32
Debug--------------------
Compiling...
xmlparse.c
c:\expat-1.95.6\source\lib\xmlparse.c(629) : error C2373:
'XML_ParserCreate' : redefinition; different type modifiers
        c:\expat-1.95.6\source\lib\expat.h(194) : see
declaration of 'XML_ParserCreate'

These errors correspond 1:1 with the public API functions
defined in expat.h. In expat.h the API functions are
declared explicitly cdecl with the XMLPARSEAPI macro. Their
definitions (in xmlparse.c for example) do not specify a
calling convention, so they follow the default for the project.

> If it works OK then we should not force any
> calling convention on the callbacks but
> simply document that the pre-built binaries
> were compiled with cdecl as default.

Currently, we're using a Perl script to patch 1.95.1 to
allow it to be compiled as fastcall and thus be compatible
with our code. We'd like to use the newer release, and for
obvious reasons, we'd like to get on the official codebase.

The "callforward" API functions define a specific calling
convention, all we need is to have one specified on the
callbacks.

Thanks,

Marsh Ray
marsh *period* ray *atsign* barrsystems *period* com


----------------------------------------------------------------------

Comment By: Karl Waclawek (kwaclaw)
Date: 2003-10-11 16:10

Message:
Logged In: YES 
user_id=290026

How does it work when you build the library
with your calling convention?

If it works OK then we should not force any
calling convention on the callbacks but simply
document that the pre-built binaries were compiled
with cdecl as default.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=310127&aid=820946&group_id=10127