Add [nostdcall] as an extended idl attribute

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Summary: Add [nostdcall] as an extended idl → Add [nostdcall] as an extended idl attribute
Created attachment 537439 [details] [diff] [review]
Patch v2

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)

Comment 2

6 years ago
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!

Comment 4

6 years ago
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?

Comment 6

6 years ago
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
Attachment #537439 - Flags: review?(khuey)
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 10

6 years ago
Comment on attachment 537439 [details] [diff] [review]
Patch v2

Please add this to the xpidl.py data model and header.py output as well.
Attachment #537439 - Flags: review?(benjamin) → review-
Blocks: 658714
Created attachment 540830 [details] [diff] [review]
Patch v3
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.

Updated

6 years ago
Attachment #540830 - Flags: review?(benjamin) → review+
Checked in, thanks for the quick review!

http://hg.mozilla.org/mozilla-central/rev/cfea84e7fd7c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.