Closed Bug 661984 Opened 13 years ago Closed 13 years ago

Add [nostdcall] as an extended idl attribute


(Core :: XPCOM, defect)

Not set





(Reporter: sicking, Assigned: sicking)




(1 file, 2 obsolete files)

Attached patch Patch to fix (obsolete) — Splinter Review
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.
Summary: Add [nostdcall] as an extended idl → Add [nostdcall] as an extended idl attribute
Attached patch Patch v2 (obsolete) — Splinter Review
This patch is tested using the patches in bug 658714 and seems to work.
Assignee: nobody → jonas
Attachment #537281 - Attachment is obsolete: true
Attachment #537439 - Flags: review?(khuey)
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.
Assignee: jonas → nobody
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!
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).
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?
Yes, on Windows they are thiscall by default.

Sure, file a new bug or coopt this one.
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.
Attachment #537439 - Flags: review?(khuey)
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.
Assignee: nobody → jonas
Comment on attachment 537439 [details] [diff] [review]
Patch v2

Kyle asked me to give this to bsmedberg since it's his final call anyway.
Attachment #537439 - Flags: review?(khuey) → review?(benjamin)
Comment on attachment 537439 [details] [diff] [review]
Patch v2

Please add this to the data model and output as well.
Attachment #537439 - Flags: review?(benjamin) → review-
Attached patch Patch v3Splinter Review
Attachment #537439 - Attachment is obsolete: true
Attachment #540830 - Flags: review?(benjamin)
Oops,  def methodReturnType(m, macro) should say |elif m.nostdcall| rather than just |if ...|. Though technically it works either way.

Fixed locally.
Attachment #540830 - Flags: review?(benjamin) → review+
Checked in, thanks for the quick review!
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.