65727 - Change from XP_PC to XP_WIN

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jon Smirl]

Could nscom.h be changed to generate a compiler error if XP_PC is used?

I recompiled an older standalone XPCOM project that had XP_PC defined. It took 
me two days of debugging to figure out what happen. 

Having XP_PC defined instead of XP_WIN causes __stdcall to be dropped from 
Add/Release calls. So everything compiles and links just fine but the app will 
give stack imbalance errors when run. In the user app the calls to Add/Release
() have been compiled as __thiscall instead of __stdcall since XP_WIN was not 
defined.

This is very difficult to find since all of the souce code looks correct when 
viewed in the debugger. Only after looking at the disassembled code could I see 
the missing push of the this pointer and finally figure out what happened.

Comment 1

[Scott Collins]

Looking for an appropriate owner for this bug.  We'll start the hunt with
Kandrot, though this may not be right.  Let's discuss and figure out who best
serves this fix.

Comment 2

[Scott Collins]

David, this is the bug we discussed.  If it's in your power to resolve this bug,
you should do it ... else you should reassign it to dveditz (now cc'd).  There
is some relation to bug #56767, currently assigned to leaf, I think.  cc'ing him
as well.

Comment 3

[timeless]

if no one minds, i'm sure i could put an error generator in nscom.h

Comment 4

[dprice (gone)]

fyi XP_PC is usedin lots of places in the code.  Removing it will require
changing XP_PC to XP_WIN

Comment 5

[Jon Smirl]

Could the test be requiring XP_PC and XP_WIN|XP_OS2? XP_PC without one of those 
two would generate an error.

Comment 6

[dprice (gone)]

handing this to dveditz, I'm not the right man for the job.

Dave

Comment 7

[John Bandhauer]

We keep seeing newsgroup postings from people who don't know why their VC 
project built binaries either don't build or don't run. Invariably they've 
failed to define XP_WIN so the xpcom method calls are declared w/o _stdcall and 
they're screwed.

I propose that we simply detect that condition and make it an #error for those 
people to fix on their own rather than them being baffled and *maybe* asking for 
unspecified help.

I think it is better to #error than to #define XP_WIN for them because we don't 
know if #defining it in the code in one place rather than in the build system 
(as the mozilla system does) won't cause some subtle and obscured error. Let's 
force them to do things right.

I'll attach a patch. 

Comment 8

[John Bandhauer]

Attachment: let's just make this an error

Comment 9

[John Bandhauer]

Looking for r= and sr= Let's just check this in and stop seeing all the 
postings from confused folks.

Comment 10

[Judson Valeski]

r=valeski

Comment 11

[Scott Collins]

sr=scc

Comment 12

[John Bandhauer]

I (finally) checked in my patch. This should deal with the immediate problem of
helping people not stumble into this hole. So, is someone really going to go
through and do the Right Thing here or should this be marked WONTFIX or futured
or what?

Comment 13

[John Bandhauer]

It turns out XP_WIN32 is needed alongside XP_WIN.
news://news.mozilla.org/9m626l%24od81%40secnews.netscape.com
http://lxr.mozilla.org/seamonkey/search?string=XP_WIN32

I'll add a new patch for nsCom.h to help people find that error - with a comment
to help them figure out what the #error means.

Comment 14

[John Bandhauer]

Attachment: Let's #error in the !defined(XP_WIN32) case too. r/sr= ?

Comment 15

[John Bandhauer]

I checked in that last patch.

Comment 16

[John Bandhauer]

Of course my patch broke the NS commercial tree where XP_WIN32 was not
#define'd. I talked it over with vidur and decided to add it to ns/config/WIN32
(XP_WIN32 does not show up anywhere in the NS commercial tree, so this should
not matter otherwise).

Fixing this bug ought to include consolidating XP_WIN and XP_WIN32. I think
we've lost the reason for this distinction (though I see we still uses of
XP_WIN16!).
http://lxr.mozilla.org/seamonkey/search?string=XP_WIN16
http://lxr.mozilla.org/seamonkey/search?string=XP_WIN32

Comment 17

[cls]

Except for NSPR & NSS, XP_PC has been removed from the mozilla tree (bug 56767).