661984 - Add [nostdcall] as an extended idl attribute

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

There really is no reason for [noscript] and [notxpcom] methods to use stdcall on windows. Ideally using those attributes should make the header generator not use those macros.

However that would be a huge change since we'd have to change a ton of function implementations to not use those macros.

A path to get there is to introduce a [notstdcall] and then migrate functions to using that, and at some point flip the default.

So here's a patch to do that!

The patch isn't tested yet so not asking for review.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

This patch is tested using the patches in bug 658714 and seems to work.

Comment 2

[Benjamin Smedberg]

Why is this important? Callee cleanup versus caller cleanup is rarely noticeable in terms of performance. And if it *is* noticeable, we should just abandon use of stdcall completely.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The triggering reason for me to do this was that it made it much easier to merge interfaces some of which were implemented in .h and some of which were implemented in .idl.

However it also keeps the code more readable when not having to use NS_IMETHOD macros.

But yes, I also thought that there was a performance difference. Doesn't stdcalls force more things to be passed on the stack rather than through registers?

Could we really abandon stdcall completely? I.e. could we deal with that complexity in xptcall? IMO that would be a big win all around!

Comment 4

[Benjamin Smedberg]

The only real difference between stdcall and cdecl is who cleans up the stack: with stdcall, the callee cleans up, and in cdecl the caller cleans up. Otherwise, the argument passing is all on the stack and the return value is always in EAX.

There is an additional difference in "thiscall" on windows, where "this" is passed in ECX instead of being a stack argument.

I really don't like this complexity in IDL: if we can remove stdcall completely we should (note that this breaks binary compatibility with MS-COM, but we've already decided that is ok).

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

so aren't all virtual methods thiscalls unless explicitly marked as stdcall? Thus declaring them as stdcalls means that there are more arguments passed on the stack rather than in registers?

If it really is an option to change xptcall to never use stdcalls then I'm all for that. (Using macros suck, and the fact that using them wrong only breaks windows sucks even more).

Want me to file a new bug?

Comment 6

[Benjamin Smedberg]

Yes, on Windows they are thiscall by default.

Sure, file a new bug or coopt this one.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 537439 [details] [diff] [review]
Patch v2

I'm assuming we no longer want this.  If that's not true, request review again.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bsmedberg: So it turns out we can't fix bug 662348 for a couple of months or so. The problem is that the accessibility code mixes XPCOM and MSCOM interfaces on the same objects which means that we can't change the XPCOM signature and still compile. The a11y code is getting fixed, but it'll take a while.

Would it be acceptable to take this patch in the meantime? I'm happy to back it out once bug 662348 has been fixed.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 537439 [details] [diff] [review]
Patch v2

Kyle asked me to give this to bsmedberg since it's his final call anyway.

Comment 10

[Benjamin Smedberg]

Comment on attachment 537439 [details] [diff] [review]
Patch v2

Please add this to the xpidl.py data model and header.py output as well.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v3

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oops,  def methodReturnType(m, macro) should say |elif m.nostdcall| rather than just |if ...|. Though technically it works either way.

Fixed locally.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in, thanks for the quick review!

http://hg.mozilla.org/mozilla-central/rev/cfea84e7fd7c