Closed Bug 478687 Opened 12 years ago Closed 11 years ago

port xpcom to 64-bit Mac OS X

Categories

(Core :: XPCOM, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(4 files, 10 obsolete files)

Attached file build error text
nspr defines uint64, which conflicts with 64-bit Mac OS X headers
I assume the answer here is not to define "uint64" in nspr if we're on 64-bit Mac OS X. When are the obsolete headers going to be removed altogether?
Blocks: 468509
> When are the obsolete headers going to be removed altogether?
After someone submits a patch that passes review.
Josh, this is a known issue.  We can't remove the
obsolete headers because of backward compatiblity.
You need to add -DNO_NSPR_10_SUPPORT to your compiler
command line.
Since this is a problem we'll need to fix in xpcom I'm just morphing this bug into a bug for porting xpcom to 64-bit Mac OS X. Hopefully that isn't too confusing given the bug summary.
Assignee: wtc → nobody
Component: NSPR → XPCOM
Product: NSPR → Core
QA Contact: nspr → xpcom
Summary: nspr defines uint64, conflicts with 64-bit Mac OS X headers → port xpcom to 64-bit Mac OS X
Version: other → Trunk
Assignee: nobody → joshmoz
Attached patch fix v0.7 (obsolete) — Splinter Review
This is a start, I'll finish this after bug 476174 is fixed. The patch there happens to remove some code I need gone.
Attached patch fix v0.8 (obsolete) — Splinter Review
This gets us a lot farther. It includes the necessary changes from bug 476174, and you'll need to be using the latest mozconfig from bug 468509. Now we're stuck on:

xpcom/reflect/xptcall/src/md/unix/xptcinvoke_unixish_x86.cpp

We'll need an alternative implementation there.
Attachment #362644 - Attachment is obsolete: true
You should be able to use the x86_64_linux.cpp variants with few or no changes.
Just a note for the future, I found this on a mysql bug and it explains many of the errors I get with x84_64_linux.cpp.

"In GNU as, "clrl/clrb %reg" are aliased to "xorl/xorb %reg, %reg", so first I'd suggest using those opcodes instead. I'd also guess that .type and .size lines can be #ifdef'd out on Darwin, since they aren't used by our object file format."
Attached patch fix v0.8.1 (obsolete) — Splinter Review
Includes Makefile changes for making 64-bit darwin use the same path as x86-64 linux in xptcall.
Attachment #362853 - Attachment is obsolete: true
Attached patch part 1, v1.0 (obsolete) — Splinter Review
Probably best to do this in parts. This stuff can land now.
Attachment #363180 - Flags: review?(mstange)
Attached patch part 1, v1.1 (obsolete) — Splinter Review
include uuid update
Attachment #363180 - Attachment is obsolete: true
Attachment #363181 - Flags: review?(mstange)
Attachment #363180 - Flags: review?(mstange)
I forgot, this can't actually land until 476174 lands, but that should be soon and we can start review now.
Don't you have to change /rdf/datasource/src/nsFileSystemDataSource.cpp and /toolkit/xre/nsCommandLineServiceMac.cpp, too?
They use NS_NewLocalFileWithFSSpec, which you're removing.

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp
...
>                 ProcessInfoRec info;
>                 info.processInfoLength = sizeof(ProcessInfoRec);
>                 info.processName = NULL;
>+#ifndef __LP64__
>                 info.processAppSpec = NULL;
>+#endif

I think you should add an #else info.processAppRef = NULL here.

The rest looks good.
Attached patch part 1, v1.2 (obsolete) — Splinter Review
Thanks for the catch Markus, I must have had other patches in my tree when I compiled.

This updated patch should work once the patch on bug 363747 lands. Easier to remove the AE code from the tree first than to fix it at this point.
Attachment #363181 - Attachment is obsolete: true
Attachment #363181 - Flags: review?(mstange)
Attached patch part 1, v1.3 (obsolete) — Splinter Review
Updated to current trunk, bug 363747 has been pushed to trunk so this should work now.
Attachment #364137 - Attachment is obsolete: true
Attachment #364926 - Flags: review?(mstange)
Attachment #364926 - Flags: review?(mstange) → review+
Attachment #364926 - Flags: superreview?(benjamin)
Attachment #364926 - Flags: superreview?(benjamin) → superreview+
Comment on attachment 364926 [details] [diff] [review]
part 1, v1.3

>diff --git a/xpcom/glue/pldhash.c b/xpcom/glue/pldhash.c

>-    table->maxAlphaFrac = (uint8)(0x100 * PL_DHASH_DEFAULT_MAX_ALPHA);
>-    table->minAlphaFrac = (uint8)(0x100 * PL_DHASH_DEFAULT_MIN_ALPHA);
>+    table->maxAlphaFrac = (PRUint8)(0x100 * PL_DHASH_DEFAULT_MAX_ALPHA);
>+    table->minAlphaFrac = (PRUint8)(0x100 * PL_DHASH_DEFAULT_MIN_ALPHA);

Changes to this pldhash.{h,c} should be in js/src/jsdhash.c, and then use plify_jsdhash.sed to get the changes into this file.

>diff --git a/xpcom/io/nsLocalFileOSX.mm b/xpcom/io/nsLocalFileOSX.mm

> NS_IMETHODIMP nsLocalFile::GetPermissions(PRUint32 *aPermissions)

>+  NSAutoreleasePool* ap = [[NSAutoreleasePool alloc] init];

Is this expensive?

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp

Please do not commit the changes to this file. A better patch is ready to land in bug 442393

sr=me on everything except the commented portions
Attached patch part 1, v1.4Splinter Review
Addresses bsmedberg's comments. Autorelease pools are cheap.
Attachment #364926 - Attachment is obsolete: true
Depends on: 442393
Attached patch part 2, v1.0 (obsolete) — Splinter Review
Mac OS X 64-bit Carbon headers don't mix well with NSPR 10 (which is deprecated). We can either undefine types all over the place, or we can change NSPR 10 on Mac OS X, but the easiest thing to do is just not use NSPR 10 in any components where we have a problem. We shouldn't be using it anywhere anyway. I already got rid of the relevant NSPR 10 dependencies in a previous patch, this patch turns NSPR 10 off where it matters.
Attachment #362940 - Attachment is obsolete: true
Attachment #366432 - Flags: review?(benjamin)
Comment on attachment 366432 [details] [diff] [review]
part 2, v1.0

Some Windows-only NSPR 10 dependencies slipped through the cracks and my try server run caught them. I'll post a new patch soon.
Attachment #366432 - Flags: review?(benjamin)
Attached patch part 2, v1.1 (obsolete) — Splinter Review
Windows build fix.
Attachment #366432 - Attachment is obsolete: true
Attachment #366517 - Flags: review?(benjamin)
Attachment #365935 - Attachment is patch: true
Attachment #365935 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 366517 [details] [diff] [review]
part 2, v1.1

It seems redundant and/or scary to be sprinkling NO_NSPR_10_SUPPORT throughout our makefiles... is there any reason we can't just AC_DEFINE it in configure?
There are still a good number of nspr 10 dependencies in our tree and I don't want to undertake that project for this patch. I think it is easy enough to search for and remove these defines when we're ready.
I don't want those defines. r=me for the code changes, but let's just avoid the defines until the entire tree has been converted.
http://mxr.mozilla.org/mozilla-central/search?string=NO_NSPR_10_SUPPORT

This link shows everything in NSPR 10 plus places where we turn NSPR 10 off. There is a lot of stuff there, and we still depend on enough of it that removing NSPR 10 deps is not trivial. I don't know exactly what we need to do because you'd have to fix the build up to any given point to know where it fails.
If we don't allow the nspr 10 defines in my patch then our options are...

- don't do any of this work until we get the whole tree off of nspr 10
- scattered type undefs all over our source files
- modify nspr 10

Adding the nspr 10 defines in my patch is the easiest thing to do now and also the easiest thing to clean up later when we can turn nspr 10 off for the whole tree. It is also the best strategy for slowly weaning our way off of nspr 10 without letting regressive code into those modules.
Comment on attachment 366517 [details] [diff] [review]
part 2, v1.1

> #  elif defined WIN32 && !defined __GNUC__
>-#   define ULL_(x)     ((uint64) x ## i64)
>+#   define ULL_(x)     ((PRUint64) x ## i64)

You can get rid of the uint64 typecast and just define it with
the ui64 suffix:

  #   define ULL_(x)     x ## ui64

-DNO_NSPR_10_SUPPORT is the right thing to do.  I did a web
search for "cssmconfig.h" and the top hits are all similar
reports of compilation errors on Leopard.  Perhaps we should
submit a bug report to Apple:
http://www.google.com/search?hl=en&q=cssmconfig.h&btnG=Google+Search&aq=f&oq=

You can also try adding

#define _UINT64

to NSPR's protypes.h, or changing the typedef to

typedef unsigned long long uint64;
One of the problem reports I found on the web suggested tweaking the
conflicting typedef to match the typedef in cssmconfig.h as a workaround.
I tried it and it indeed works.

In general we can't change typedefs like this in NSPR because of our
backward compatibility requirements, even though 'unsigned long'
and 'unsigned long long' are the same size.  But we're lucky -- no
NSPR release supports 64-bit Mac OS X, so we can make this change
now.

In the comment I use CoreServices.h as a representative Carbon header
that includes cssmconfig.h.  If there is a better Carbon header to use as
an example, please let me know.
Attachment #368201 - Flags: review?(joshmoz)
Wan-Teh, thanks for the patch! Unfortunately it doesn't work for what I need. The compiler still does not like the conflicting definition.

error: conflicting declaration ‘typedef uint64_t uint64’
error: ‘uint64’ has a previous declaration as ‘typedef PRUint64 uint64’

Like you said, I still think -DNO_NSPR_10_SUPPORT is the right thing to do. I'd rather not get into modifying protypes.h.
Attachment #368201 - Flags: review?(joshmoz)
Josh, I made a mistake in the patch.  Please change
    DARWIN
to
    __APPLE__
in the #if test.  Thanks.
That worked, excellent. This patch applies on Mozilla trunk and includes the __APPLE__ fix.

When do you think we could get an nspr with this fix merged to Mozilla trunk?
Attachment #368309 - Flags: review+
Attachment #366517 - Flags: review?(benjamin)
Comment on attachment 366517 [details] [diff] [review]
part 2, v1.1

Josh, thanks for testing my patch.  I can push a new NSPR tag to
mozilla-central this weekend.

You should still do the nsIStreamBufferAccess.idl cleanup in this
patch:

> #  elif defined WIN32 && !defined __GNUC__
>-#   define ULL_(x)     ((uint64) x ## i64)
>+#   define ULL_(x)     ((PRUint64) x ## i64)

Either your change, or my suggestion of x ## ui64, would be fine.
I also suggest that you change the ifdef to

  #  elif defined WIN32 && defined _MSC_VER
Yeah, I'm doing a test build with "ui64" right now and I'll land the change soon.
Do you know why my patch works, but if you compile a simple file
containing two identical typedefs:

typedef int int32;
typedef int int32;

it doesn't work:

bar.c:2: error: redefinition of typedef int32
bar.c:1: error: previous declaration of int32 was here
Attachment #366517 - Attachment is obsolete: true
Attachment #368201 - Attachment is obsolete: true
Attachment #368342 - Flags: review+
Comment on attachment 368342 [details] [diff] [review]
nsIStreamBufferAccess fix v1.0

r=wtc.
Attachment #368342 - Flags: review?(benjamin)
Comment on attachment 368309 [details] [diff] [review]
wtc fix v1.1 (checked in)

I checked in this patch on the NSPR trunk (NSPR 4.8).

Checking in nsprpub/pr/include/prtypes.h;
/cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v  <--  prtypes.h
new revision: 3.39; previous revision: 3.38
done
Attachment #368309 - Attachment description: wtc fix v1.1 → wtc fix v1.1 (checked in)
Comment on attachment 368309 [details] [diff] [review]
wtc fix v1.1 (checked in)

I pushed this NSPR patch to mozilla-central in
changeset 956071116564:
http://hg.mozilla.org/mozilla-central/rev/956071116564
Attachment #368342 - Flags: review?(benjamin) → review+
pushed "nsIStreamBufferAccess fix v1.0" to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/fb5635f0d184
Depends on: 485049
Now that bug 485049 has been fixed we can close this.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.