Last Comment Bug 661984 - Add [nostdcall] as an extended idl attribute
: Add [nostdcall] as an extended idl attribute
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking)
:
Mentors:
Depends on:
Blocks: 658714
  Show dependency treegraph
 
Reported: 2011-06-03 17:40 PDT by Jonas Sicking (:sicking)
Modified: 2011-06-23 20:08 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (5.53 KB, patch)
2011-06-03 17:40 PDT, Jonas Sicking (:sicking)
no flags Details | Diff | Review
Patch v2 (7.49 KB, patch)
2011-06-05 04:12 PDT, Jonas Sicking (:sicking)
benjamin: review-
Details | Diff | Review
Patch v3 (12.22 KB, patch)
2011-06-21 12:12 PDT, Jonas Sicking (:sicking)
benjamin: review+
Details | Diff | Review

Description Jonas Sicking (:sicking) 2011-06-03 17:40:20 PDT
Created attachment 537281 [details] [diff] [review]
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) 2011-06-05 04:12:22 PDT
Created attachment 537439 [details] [diff] [review]
Patch v2

This patch is tested using the patches in bug 658714 and seems to work.
Comment 2 Benjamin Smedberg [:bsmedberg] 2011-06-06 07:14:31 PDT
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) 2011-06-06 08:19:37 PDT
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 [:bsmedberg] 2011-06-06 08:39:28 PDT
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) 2011-06-06 08:49:20 PDT
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 [:bsmedberg] 2011-06-06 11:13:29 PDT
Yes, on Windows they are thiscall by default.

Sure, file a new bug or coopt this one.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-06-07 16:38:11 PDT
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) 2011-06-08 17:37:56 PDT
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) 2011-06-16 11:59:43 PDT
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 [:bsmedberg] 2011-06-17 09:55:24 PDT
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) 2011-06-21 12:12:47 PDT
Created attachment 540830 [details] [diff] [review]
Patch v3
Comment 12 Jonas Sicking (:sicking) 2011-06-21 12:37:46 PDT
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) 2011-06-23 20:08:05 PDT
Checked in, thanks for the quick review!

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

Note You need to log in before you can comment on or make changes to this bug.