Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

Assignee

Description

11 years ago
Posted file build error text
nspr defines uint64, which conflicts with 64-bit Mac OS X headers
Assignee

Comment 1

11 years ago
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?
Assignee

Updated

11 years ago
Blocks: 468509
> When are the obsolete headers going to be removed altogether?
After someone submits a patch that passes review.

Comment 3

11 years ago
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.
Assignee

Comment 4

11 years ago
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

Updated

11 years ago
Assignee: nobody → joshmoz
Assignee

Comment 5

11 years ago
Posted 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.
Assignee

Comment 6

11 years ago
Posted 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

Comment 7

11 years ago
You should be able to use the x86_64_linux.cpp variants with few or no changes.
Assignee

Comment 8

11 years ago
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."
Assignee

Comment 9

11 years ago
Posted 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
Assignee

Comment 10

11 years ago
Posted 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)
Assignee

Comment 11

11 years ago
Posted 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)
Assignee

Comment 12

11 years ago
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.
Assignee

Comment 14

10 years ago
Posted 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)
Assignee

Comment 15

10 years ago
Posted 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+
Assignee

Updated

10 years ago
Attachment #364926 - Flags: superreview?(benjamin)

Updated

10 years ago
Attachment #364926 - Flags: superreview?(benjamin) → superreview+

Comment 16

10 years ago
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
Assignee

Comment 17

10 years ago
Posted patch part 1, v1.4Splinter Review
Addresses bsmedberg's comments. Autorelease pools are cheap.
Attachment #364926 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Depends on: 442393
Assignee

Comment 19

10 years ago
Posted 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)
Assignee

Comment 20

10 years ago
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)
Assignee

Comment 21

10 years ago
Posted patch part 2, v1.1 (obsolete) — Splinter Review
Windows build fix.
Attachment #366432 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Attachment #366517 - Flags: review?(benjamin)
Assignee

Updated

10 years ago
Attachment #365935 - Attachment is patch: true
Attachment #365935 - Attachment mime type: application/octet-stream → text/plain

Comment 22

10 years ago
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?
Assignee

Comment 23

10 years ago
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.

Comment 24

10 years ago
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.
Assignee

Comment 25

10 years ago
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.
Assignee

Comment 26

10 years ago
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 27

10 years ago
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;

Comment 28

10 years ago
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)
Assignee

Comment 29

10 years ago
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.
Assignee

Updated

10 years ago
Attachment #368201 - Flags: review?(joshmoz)

Comment 30

10 years ago
Josh, I made a mistake in the patch.  Please change
    DARWIN
to
    __APPLE__
in the #if test.  Thanks.
Assignee

Comment 31

10 years ago
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+
Assignee

Updated

10 years ago
Attachment #366517 - Flags: review?(benjamin)

Comment 32

10 years ago
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
Assignee

Comment 33

10 years ago
Yeah, I'm doing a test build with "ui64" right now and I'll land the change soon.

Comment 34

10 years ago
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
Assignee

Comment 35

10 years ago
Assignee

Updated

10 years ago
Attachment #366517 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Attachment #368201 - Attachment is obsolete: true

Updated

10 years ago
Attachment #368342 - Flags: review+

Comment 36

10 years ago
Comment on attachment 368342 [details] [diff] [review]
nsIStreamBufferAccess fix v1.0

r=wtc.
Assignee

Updated

10 years ago
Attachment #368342 - Flags: review?(benjamin)

Comment 37

10 years ago
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 38

10 years ago
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

Updated

10 years ago
Attachment #368342 - Flags: review?(benjamin) → review+
Assignee

Comment 39

10 years ago
pushed "nsIStreamBufferAccess fix v1.0" to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/fb5635f0d184
Assignee

Updated

10 years ago
Depends on: 485049
Assignee

Comment 40

10 years ago
Now that bug 485049 has been fixed we can close this.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.