229722 - 64bit safe code for mozilla/content (WinXP AMD64)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Makoto Kato [:m_kato]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031208
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031208

Curretly code is defined as pointer type.

mozilla/content/base/public/nsIContent.h
...
  #endif

  protected:
    typedef long PtrBits;
    ^^^^^^^^^^^^^^^^^^^^^

     // Subclasses may use the low two bits of mParentPtrBits to store other data
     enum { kParentBitMask = 0x3 };
...

But WinXP 64bits for AMD64 is that "long" type is defined as 32 bits, so pointer
cast is always invalied.  So you should replace it with PRWord.  (PRWord is
pointer value).

Reproducible: Always

Steps to Reproduce:
not need.  This is porting issue
Actual Results:  
not need.  This is porting issue

Expected Results:  
not need.  This is porting issue

Comment 1

[Makoto Kato [:m_kato]]

Attachment: mozilla/content diffs

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

PRWord is currently typedef-ed to |long|, so I don't see how this will help.

Comment 3

[Boris Zbarsky [:bzbarsky]]

In any case, this is jst/peterv stuff, not misc layout code.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Perhaps PRWord should be typedeffed to long long as needed (a configure check)?

Comment 5

[Makoto Kato [:m_kato]]

please see bug #226094 and bug #225859 about pointer structure of PR* and js*.

Comment 6

[timeless]

http://landfill.mozilla.org/mxr-test/nspr/source/nsprpub/pr/include/prtypes.h#472
462 ** WARNING: The undocumented data types PRWord and PRUword are
463 ** only used in the garbage collection and arena code.  Do not
464 ** use PRWord and PRUword in new code.
465 **
466 ** A PRWord is an integer that is the same size as a void*.
467 ** It implements the notion of a "word" in the Java Virtual
468 ** Machine.  (See Sec. 3.4 "Words", The Java Virtual Machine
469 ** Specification, Addison-Wesley, September 1996.
470 ** http://java.sun.com/docs/books/vmspec/index.html.)
471 */
472 typedef long PRWord;
473 typedef unsigned long PRUword;

Comment 7

[Brendan Eich [:brendan]]

First, this bug should not depend on bug 226094.  Rather, all of this bug, that
bug, and bug 225859 should depend on a meta-bug about supporting LLP64
compilation targets (of which WinXP on AMD64 is the first to come to our attention).

Second, let's fix this right, once.  We should use PRWord (PRUword if signed
comparison or sign extension would do the wrong thing) consistently in our code,
through whatever local typedef names are appropriate.  But this requires
changing NSPR, and changing deprecated NSPR types, to boot.  Cc'ing Wan-Teh.

/be

Comment 8

[Brendan Eich [:brendan]]

So js/src/jstypes.h is an old, ad-hoc fork of prtypes.h, done to make JS stand
alone whether built into Mozilla or not.  Therefore we'd need autoconf tests for
two places: NSPR and JS.  On the plus side, the JSWord (jsword, using the older
scalar-typedef lowercase naming convention) type and its unsigned counterpart
are not deprecated.

Wan-Teh, can we un-deprecate PRWord/PRUword and parameterize them for LLP64 as
well as LP64?  I hope you're reading bugmail -- if so, Happy New Year.

/be

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 138166 [details] [diff] [review]
mozilla/content diffs

r+sr=jst

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that there are other places that use the same typedef. Both the nsAttrValue
and nsAttrAndChildArray does.

Comment 11

[Makoto Kato [:m_kato]]

checked in

Comment 12

[Boris Zbarsky [:bzbarsky]]

Er.... we're in freeze.  Checkins require driver approval, no?

Comment 13

[Johnny Stenback (:jst)]

Yes, Makoto, this shouldn't have been checked in w/o approval from drivers. In
the future, pay attention to the rules. (for this particular change, I don't
think it's worth backing out or even worrying drivers with it since it really
doesn't change anything on the tier-1 platforms).

Comment 14

[Wan-Teh Chang]

Re: comment 7 and comment 8

As long as we precisely define PRWord and PRUword,
that should be fine.  I think what you want is the
new intptr_t and uintptr_t types in C99.

What I don't like about PRWord and PRUword are:

1. "Word" has many meanings.  To the best of my
knowledge, the "word" in PRWord means a word in
the Java virtual machine (the predecessor of NSPR
was used by Netscape to port Sun's JVM to other
platforms), and most people don't know what exactly
a JVM word means.

2. PRWord is defined to be an integer that is the
same size as a void*.  "The same size" might be
overly restrictive.  It may be better to say "large
enough to hold".

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> 2. PRWord is defined to be an integer that is the
> same size as a void*.  "The same size" might be
> overly restrictive.  It may be better to say "large
> enough to hold".

Actually, in many cases we do want something that is "The same size". "large
enough to hold" might mean that it's bigger then a void* which might cause
unneccesary bloat.

Does intptr_t and uintptr_t work across enough compilers for us to use in XP
mozilla code? If not we *need* something that is defined to be "The same size"
as a void* or we'll keep running into this problem.
Note that it's still not fixed for the nsAttrValue and nsAttrName classes.

Comment 16

[Makoto Kato [:m_kato]]

About discuss PRWord value, please see bug 226094 and bug 225859.  Since PRWord
is defined as pointer, we should use it, not long.

And about other code of Mozilla/content, I file a new bug as bug 239499.

Comment 17

[Wan-Teh Chang]

Jonas: intptr_t and uintptr_t are relatively new and
therefore are not universally supported yet.  Also,
they are "large enough to hold", as opposed to "the
same size as", a pointer.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Then it sounds like intptr_t and uintptr_t won't cut it here and we need some
NSPR typedef that we can rely on.