Last Comment Bug 740688 - Use uintptr_t instead of PRUword, and intptr_t instead of PRWord
: Use uintptr_t instead of PRUword, and intptr_t instead of PRWord
Status: RESOLVED FIXED
[good first bug][mentor=Waldo][lang=c++]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla14
Assigned To: Mark Capella [:capella]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-29 18:41 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-04-14 06:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (30.75 KB, patch)
2012-04-04 14:46 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (30.99 KB, patch)
2012-04-06 19:07 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (38.72 KB, patch)
2012-04-10 16:41 PDT, Mark Capella [:capella]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-29 18:41:39 PDT
Per NSPR, "A PRWord is an integer that is the same size as a void*", and mutatis mutandis for PRUword.  This is straightforwardly the same as intptr_t and uintptr_t.  We should use those *standard* types (found in #include "mozilla/StandardInteger.h") rather than the PR{Uw,W}ord types -- which, further, are accompanied by this lovely comment:

502 ** WARNING: The undocumented data types PRWord and PRUword are
503 ** only used in the garbage collection and arena code.  Do not
504 ** use PRWord and PRUword in new code.

Note that uses in nspr/pr shouldn't be changed -- NSPR is its own project, we don't modify it, and they use obfuscatory types more readily than we do these days.  The ipc/chromium file probably should have its code for these types removed -- they're not used in the code we imported, and our policy (for better or worse) is that we don't care that we've forked that code.  Finally, security/nss shouldn't be changed, because NSS is also its own project.  (It probably shouldn't be using PRWord either, but at the very least that should be changed in a separate bug.)
Comment 1 Mark Capella [:capella] 2012-04-04 08:55:05 PDT
I don't see anyone assigned to this. If you have no objections I'll work on it.

I scanned for PRWord and PRUword type keywords, and removing the files in folders nspr/pr and security/nss, I wind up the following list of affected files:

/ipc/chromium/src/base/third_party/nspr/prtypes.h

/caps/include/nsScriptSecurityManager.h
/caps/src/nsScriptSecurityManager.cpp

/content/xslt/src/xpath/txMozillaXPathTreeWalker.cpp

/dom/plugins/base/nsNPAPIPluginStreamListener.cpp

/layout/base/nsDisplayList.cpp
/layout/base/nsDisplayList.h
/layout/base/nsPresArena.cpp
/layout/base/nsPresArena.h
/layout/generic/nsGfxScrollFrame.cpp
/layout/style/nsRuleData.cpp
/layout/style/nsRuleNode.h

/js/xpconnect/src/XPCWrappedNative.cpp
/js/xpconnect/src/xpcprivate.h

/xpcom/base/nsCycleCollector.cpp
/xpcom/base/nscore.h
/xpcom/glue/nsVoidArray.h
/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_mips.cpp
/xpcom/reflect/xptcall/src/md/unix/xptcstubs_mips.cpp
Comment 2 Mark Capella [:capella] 2012-04-04 14:46:06 PDT
Created attachment 612351 [details] [diff] [review]
Patch (v1)

Well I had some abndwitdh available so I went and coded / built / tested this attached patch locally. It all looks good.

I tried to remove the PR{Uw,W}ord type definitions from the /ipc/chromium/src/base/third_party/nspr/prtypes.h file, but that caused a build failure centered around C:\Users\Master\mozilla-central\nsprpub\pr\include\obsolete\protypes.h so I left it in place.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-06 15:08:47 PDT
Comment on attachment 612351 [details] [diff] [review]
Patch (v1)

Review of attachment 612351 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/include/nsScriptSecurityManager.h
@@ +146,5 @@
>  
>  // Property Policy
>  union SecurityLevel
>  {
> +    intptr_t   level;

Preserve alignment here, since the old code did.

::: caps/src/nsScriptSecurityManager.cpp
@@ +3341,1 @@
>                   "This may cause a security failure with the SecurityLevel union.");

I bet this predated MOZ_STATIC_ASSERT.  Switch to that here?

::: js/xpconnect/src/xpcprivate.h
@@ +2850,5 @@
>      XPCNativeSet*                mSet;
>      JSObject*                    mFlatJSObject;
>      XPCNativeScriptableInfo*     mScriptableInfo;
>      XPCWrappedNativeTearOffChunk mFirstChunk;
> +    intptr_t                       mWrapperWord;

Alignment.

::: layout/style/nsRuleData.cpp
@@ +48,5 @@
>    // offsets.
> +  MOZ_STATIC_ASSERT(sizeof(uintptr_t) == sizeof(size_t),
> +                    "expect uintptr_t and size_t to be the same size");
> +  MOZ_STATIC_ASSERT(uintptr_t(-1) > uintptr_t(0),
> +                    "expect uintptr_t to be unsigned");

Arguably we could just remove this, since uintptr_t is a standard type, and if it were broken a whole lot of other things would be broken besides this.  But it doesn't really hurt, either.  And I guess we're doing the same thing for size_t below, so meh.

::: xpcom/base/nscore.h
@@ +394,5 @@
>   */
>  
> +#define NS_PTR_TO_INT32(x)  ((PRInt32)  (intptr_t) (x))
> +#define NS_PTR_TO_UINT32(x) ((PRUint32) (intptr_t) (x))
> +#define NS_INT32_TO_PTR(x)  ((void *)   (intptr_t) (x))

Ugh these macros are so anachronistic.  I guess we shouldn't do anything about them here, tho.
Comment 4 Mark Capella [:capella] 2012-04-06 19:07:11 PDT
Created attachment 613056 [details] [diff] [review]
Patch (v2)

Nits addressed ... builds and tests locally.
Comment 5 Mozilla RelEng Bot 2012-04-09 07:08:14 PDT
Autoland Patchset:
	Patches: 613056
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=2a51ac9edfb2
Try run started, revision 2a51ac9edfb2. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=2a51ac9edfb2
Comment 6 Mark Capella [:capella] 2012-04-10 16:41:49 PDT
Created attachment 613824 [details] [diff] [review]
Patch (v3)
Comment 7 Mozilla RelEng Bot 2012-04-10 16:48:02 PDT
Autoland Patchset:
	Patches: 613824
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=d88dd126c9bc
Try run started, revision d88dd126c9bc. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=d88dd126c9bc
Comment 8 Mozilla RelEng Bot 2012-04-10 21:01:23 PDT
Try run for d88dd126c9bc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d88dd126c9bc
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-d88dd126c9bc
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-11 17:19:42 PDT
Comment on attachment 613824 [details] [diff] [review]
Patch (v3)

Review of attachment 613824 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good modulo one super-nit.  I'll fix that and land this, probably tomorrow.  Thanks!

::: js/xpconnect/src/xpcprivate.h
@@ +48,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/StandardInteger.h"

This should be before Util.h to be alphabetical.
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-12 22:24:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/622e0d1cc986

Good stuff!  More NSPR craziness dies a painful death.
Comment 11 Marco Bonardo [::mak] 2012-04-14 06:19:56 PDT
https://hg.mozilla.org/mozilla-central/rev/622e0d1cc986

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