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

SourceForge.net noreply at sourceforge.net
Tue Oct 14 17:35:40 EDT 2003


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-14 17:35

Message:
Logged In: YES 
user_id=3066

Ok, here's what I propose doing:

- add a macro, XMLCALL, that expands to whatever is needed
to nail down the calling convention for all calls across the
library boundary
- add the XMLCALLBACK macro, defined to use XMLCALL
- add XMLCALL to XMLPARSEAPI
- modify all example code to include XMLCALL for definitions
of callbacks
- document, document, document!

Any of these can be overridden before including expat.h, for
those who are adventurous.  For those who stick to their
compiler defaults, there should be no perceivable change.

Once done, I'll release another snapshot, with emails sent
to relevant lists (expat-discuss, Python's xml-sig; feel
free to suggest others, or just pass the word when the
snapshot's up).

I've started the code changes based on this patch, but I've
only tested using GCC 3.2.2 on RedHat 9 so far.

Comments?

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

Comment By: Karl Waclawek (kwaclaw)
Date: 2003-10-13 19:29

Message:
Logged In: YES 
user_id=290026

I am a little late with my replies.
So, summarizing I would say:

Let's force cdecl on all API calls, call-back or not.

About performance:
Considering how much work Expat does behind each
call-back the calling convention will likely not have
a measurable impact. I agree with Fred here.
Also, looking back at our own testing with internal
calling conventions, cdecl came out ahead. In my opinion,
and I think in Fred's opinion too, this was because
cdecl allows optimizations for call "clusters" as the caller
manages the stack, not the callee.
I really don't think fastcall will give any measurable
advantage, even for the call-backs.

So, using this patch, what else is there to do?
Should we be concerned with memory handler calls
and whatever else will have an undefined calling convention?

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

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

Message:
Logged In: YES 
user_id=3066

Marsh:
 > By 'frozen' I assume you mean using the default calling
 > conventions specified at compile time?

Yes.

 > I haven't seen any warnings from my patch to expat.h (with
 > the client code and MSVC 7.1). Is it possible that a
 > compiler upgrade has the compiler issue?

I would have been using MSVC 6, since that's the only MSVC
I've had for years.  Not sure what service pack, though. 
Also, I'm no Windows expert, mostly only using MSVC in a
mindless, repetitive "did that break something" mode.  I'll
play with your patch with actual compilers, though, and see
how well I survive.  ;-)

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

Comment By: Marsh Ray (marshray_bs)
Date: 2003-10-13 17:54

Message:
Logged In: YES 
user_id=883966

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

> 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.

Agreed, although this is not exactly the problem I was
intending to address. We have both cdecl and fastcall
default calling conventions being used in our processes.
Currently, we're using multiple expat.dll's to handle this.
It would be nice to be able to use the same expat.dll
everywhere to reduce build-time complexity and run-time
footprint.
Ridding our code of Xerces is now perhaps another matter . .
. :)

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

By 'frozen' I assume you mean using the default calling
conventions specified at compile time?

> 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 mefreeze the
> if I am missing some). I think we can non-performancemuch
> problems, sensitive API without but we should leave the
> callbacks open for optimization.

When XML becomes a performance bottleneck, we would
typically change our architecture to use faster binary
structures. Therefore the ~5% gain in XML parsing we might
get by compiling Expat as fastcall is not really
significant. Our main goal is to get everyone on the same .dll.

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

Sure, if you want to require people to compile a different
version of expat.dll for every default calling convention in
use.
I think the solution for people who want to compile expat as
fastcall (if that person exists) would be to allow them to
set a #define (in their client code) to specify the calling
convention that their custom-compiled dll wants.

For example:
#define XML_EXPAT_USES_FASTCALL 1 /* or _STDCALL, _CDECL is
default */
#include "expat.h" /* be sure to link with
expat_fastcall.lib and .dll */

Again, this ability is not particularly useful to us.

> 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.

>Comment By: Fred L. Drake, Jr. (fdrake)
> 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!

In my Microsoft headers, I find:
_CRTIMP void *  __cdecl malloc(size_t);

Furthermore, there is not a different C runtime library for
each default calling convention.

So, it's probably safe to assume malloc is going to be
cdecl. Stating that explicitly in the definiton of
XML_Memory_Handling_Suite should work, although it looks
like there'll be a few places to change in xmlparse.c.

#ifndef XMLCDECL
#if defined(_MSC_EXTENSIONS) && !defined(__BEOS__) &&
!defined(__CYGWIN__)
#define XMLCDECL __cdecl
#else
#define XMLCDECL
#endif
#endif  /* not defined XMLCDECL*/

typedef struct {
  void *(XMLCDECL *malloc_fcn)(size_t size);
  void *(XMLCDECL *realloc_fcn)(void *ptr, size_t size);
  void (XMLCDECL *free_fcn)(void *ptr);
} XML_Memory_Handling_Suite;

>Comment By: Karl Waclawek (kwaclaw)
> 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?

I would guess the author simply had a habit of being hygenic
and didn't want unused defines to propagate out of the
inclusing of expat.h.

Gee, as I am writing this, you guys keep posting new stuff.

> 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.

I haven't seen any warnings from my patch to expat.h (with
the client code and MSVC 7.1). Is it possible that a
compiler upgrade has the compiler issue?

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



More information about the Expat-bugs mailing list