#64 expat.h not great for MSVC !cdecl deflt calling cnvntn

closed-accepted
None
7
2003-10-15
2003-10-09
Anonymous
No

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

Discussion

  • Nobody/Anonymous

    Patch to expat.h.

     
  • Karl Waclawek

    Karl Waclawek - 2003-10-11

    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.

     
  • Marsh Ray

    Marsh Ray - 2003-10-13

    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

     
  • Fred L. Drake, Jr.

    • priority: 5 --> 6
     
  • Fred L. Drake, Jr.

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

     
  • Karl Waclawek

    Karl Waclawek - 2003-10-13

    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

     
  • Fred L. Drake, Jr.

    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!

     
  • Fred L. Drake, Jr.

    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.

     
  • Marsh Ray

    Marsh Ray - 2003-10-13

    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

     
  • Fred L. Drake, Jr.

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

     
  • Karl Waclawek

    Karl Waclawek - 2003-10-13

    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?

     
  • Fred L. Drake, Jr.

    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?

     
  • Marsh Ray

    Marsh Ray - 2003-10-14

    Logged In: YES
    user_id=883966

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

    Sounds good to me.

    > - document, document, document!

    If the changes are made as described above, it won't affect
    any previously functioning code. All that needs to be
    documented is the new functionality.

    A callback function defined in a module with a default
    calling convention other than cdecl will now need to
    explicitly specify the calling convention that is expected
    by expat (typcially cdecl).

    For example:
    void myStartElementHandler(
    void *userData, const XML_Char *name, const XML_Char **atts);

    Becomes:
    void __cdecl myStartElementHandler(
    void *userData, const XML_Char *name, const XML_Char **atts);

     
  • Karl Waclawek

    Karl Waclawek - 2003-10-14

    Logged In: YES
    user_id=290026

    Fine with me, Fred.

     
  • Fred L. Drake, Jr.

    Logged In: YES
    user_id=3066

    Assigned to myself; I think I'm almost done with the code,
    and have started on the documentation. Bumped priority to
    reflect intention to include this in 1.95.7.

    At least for some Unix platforms, which use standard headers
    that assume cdecl calling convention as the default, Expat
    itself must be compiled with cdecl as the default.
    (Otherwise malloc() & friends are not cdecl, and specifying
    cdecl in the mem functions causes type errors at compile
    time.) This should not affect code outside of Expat itself.

     
  • Fred L. Drake, Jr.

    • priority: 6 --> 7
    • assigned_to: nobody --> fdrake
     
  • Fred L. Drake, Jr.

    Logged In: YES
    user_id=3066

    After working on this some more and trying to explain the
    macros, I'm going to revise the proposed change a little.

    - add a macro, XMLCALL, that expands to whatever is needed
    to nail down the calling convention for all calls across the
    library boundary
    - another new macro, XMLIMPORT, defined to be whatever magic
    is needed to mark an entry point as imported from a
    dynamically loaded module (.dll, .so, .sl, whatever)
    - use XMLCALL and XMLIMPORT to define XMLPARSEAPI
    - modify all example code to include XMLCALL for definitions
    of callbacks
    - modify callback typedefs to use XMLCALL
    - document, document, document!

    This proves just a bit simpler in the mechanics of it all,
    and keeps the layers of magic down. Since the XMLCALLBACK
    really only adds XMLCALL and an asterisk, it can be
    completely dropped since XMLCALL is available.

    Marsh: Yes, I agree that no changes are needed for
    existing, working code. These changes are entirely to
    benefit people who want to use a default calling convention
    other than cdecl.

    No need for followup comments; watch for commits and a new
    snapshot later today (possibly *much* later).

     
  • Fred L. Drake, Jr.

    • status: open --> open-accepted
     
  • Fred L. Drake, Jr.

    • status: open-accepted --> closed-accepted
     
  • Fred L. Drake, Jr.

    Logged In: YES
    user_id=3066

    Changes committed as:

    doc/reference.html 1.49
    examples/elements.c 1.4
    examples/outline.c 1.5
    lib/expat.h 1.55
    lib/xmlparse.c 1.112
    tests/runtests.c 1.53
    xmlwf/xmlwf.c 1.66

    Closing this report.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks