303345 - add Universal Binary (mach-o FAT) plugin support

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

We crash trying to use Universal Binary plugins on Mac OS X. We need to learn to
deal with them.

Comment 1

[Josh Aas]

Attachment: prlink mac cleanup, v1.0

This is a somewhat large patch, but it does only one specific thing - removes
XP_MAC code from prlink.c. Should be easy to review. I have tested it,
everything seems to work. The #idfef stuff in prlink.c is really confusing, and
removing it will make future patches easier to write and review.

Comment 2

[Mark Mentovai]

Comment on attachment 191618 [details] [diff] [review]
prlink mac cleanup, v1.0

If it's appropriate to remove XP_MAC from NSPR, I've got two suggestions:

Get rid of this:
 #define PStrFromCStr(src, dst) c2pstrcpy(dst, src)

and just use c2pstrcpy directly (remember to swap the arguments).

and

+#if defined(XP_MACOSX)

Use #ifdef x as shorthand for #if defined(x) when the logic is no more complex
than this.

Comment 3

[Josh Aas]

Comment on attachment 191618 [details] [diff] [review]
prlink mac cleanup, v1.0

For some reason this patch won't apply any more for me. Cancelling review.

Comment 4

[Wan-Teh Chang]

Comment on attachment 191618 [details] [diff] [review]
prlink mac cleanup, v1.0

I already reviewed this patch.	I will be happy to check it
in after you get any necessary approval.  I'd like to make
some additional changes.

1. Remove some blank lines that were there to separate the
#ifdef blocks.	Now that the #ifdef's are moved, those blank
lines are no longer necessary.

2. Remove a block of comment in pr_LoadViaCFM that no longer
makes sense because the comment ("The name did contain a ":",
so it must be a full path name. ...") refers to code that has
been removed.

3. In pr_LoadViaCFM, remove unnecessary braces and move the code
to the left.

I want you to review another change I want to make:

> static PRStatus
> pr_LoadViaCFM(const char *name, PRLibrary *lm)
> {
>     OSErr err;
>     char cName[64];
...
>         p2cstrcpy(cName, fileSpec.name);
...
>     return (err == noErr) ? PR_SUCCESS : PR_FAILURE;
> }

I believe "cName" can be removed because it is created
(by the p2cstrcpy call) but not used.  Do you agree?

Comment 5

[Wan-Teh Chang]

I will make the two changes that Mark suggested in comment 2.

Comment 6

[Wan-Teh Chang]

Attachment: Mark and Wan-Teh's suggested changes (cvs diff -w)

OK, here is a patch (whitespace ignored, so the indentation
of some code won't look right) that will help you understand
my suggested changes.  It also has the two changes Mark
suggested in comment 2.

Note that I expand SYM_OFFSET to 1 because NEED_LEADING_UNDERSCORE
is defined for DARWIN, hence for XP_MACOSX.

Comment 7

[Wan-Teh Chang]

Attachment: Patch for prlink.h to reflect removed code

This patch adds comments to the obsolete enum constants
and union members that are only used by Mac OS Classic.
We can't remove the obsolete union members because that
will break binary compatibility (the union's size will
change).

Comment 8

[Josh Aas]

Comment on attachment 191756 [details] [diff] [review]
Mark and Wan-Teh's suggested changes (cvs diff -w)

Looks good to me. So you will land both my patch (do you need a new one?) and
yours once I get approval for them?

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 191760 [details] [diff] [review]
Patch for prlink.h to reflect removed code

Strictly speaking, the comments should refer to "Mac CFM", not "Mac OS Classic"

Comment 10

[Simon Fraser [no longer active]]

Comment on attachment 191756 [details] [diff] [review]
Mark and Wan-Teh's suggested changes (cvs diff -w)

>--- prlink-josh.c	2005-08-05 17:12:53.968750000 -0700
>+++ prlink.c	2005-08-05 17:13:45.812500000 -0700

>-#if defined(XP_MACOSX)
>+#ifdef XP_MACOSX
> #include <CodeFragments.h>
> #include <TextUtils.h>
> #include <Types.h>
> #include <Aliases.h>
>-
> #include <CFURL.h>
> #include <CFBundle.h>
> #include <CFString.h>
> #include <CFDictionary.h>
> #include <CFData.h>

Replace all these with <Carbon/Carbon.h> ?

>-#if defined(XP_MACOSX)
>-#if defined(NEED_LEADING_UNDERSCORE)
>-#define SYM_OFFSET 1
>-#else
>-#define SYM_OFFSET 0
>-#endif
>+#ifdef XP_MACOSX
>     if (lm->bundle) {
>-        CFStringRef nameRef = CFStringCreateWithCString(NULL, name + SYM_OFFSET, kCFStringEncodingASCII);
>+        CFStringRef nameRef = CFStringCreateWithCString(NULL, name + 1, kCFStringEncodingASCII);

I'd like to see a comment explaining "name + 1". I presume this is to skip the
underscore? Have we already tested that strlen(name) > 1 ?

Comment 11

[Josh Aas]

This bug needs to be a blocker. We will need to do this on the branch at some
point, since x86 Macs and plugins for them will be shipping before we move off
of the branch. If plugin vendors move to UB plugins, we will be broken on PPC as
well as x86. I'm working on this now.

Comment 12

[Josh Aas]

Attachment: fix v1.0

Don't use CFM function pointers on Mac OS X x86. Allows UB plugins to work on
x86.

Comment 13

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 191903 [details] [diff] [review]
fix v1.0

r=pink

Comment 14

[Simon Fraser [no longer active]]

Comment on attachment 191903 [details] [diff] [review]
fix v1.0

So in tree I see __POWERPC__ and __powerpc__, but not __PowerPC__. Which is it?

Comment 15

[Josh Aas]

Its POWERPC, or at least that has been tested and works for me. I'll fix casing
on checkin.

Comment 16

[Josh Aas]

I should note though, that both __PowerPC__ and __POWERPC__ work for me, so my
patch isn't technically wrong.

Comment 17

[Simon Fraser [no longer active]]

Comment on attachment 191903 [details] [diff] [review]
fix v1.0

sr with

+#ifdef XP_MACOSX && __PowerPC__

changed to 

+#if defined(XP_MACOSX) && defined(__POWERPC__)

Comment 18

[Mark Mentovai]

(In response to comment 14:)
> So in tree I see __POWERPC__ and __powerpc__, but not __PowerPC__. Which is it?

__ppc__ or __POWERPC__.  I prefer the former, it's more modern.  __powerpc__
doesn't work for Macs, only other operating systems.  __PowerPC__ is just broken.

Comment 19

[Josh Aas]

__ppc__ never works for me - I've tried it at your request a few times already,
so we can't do that

Comment 20

[Josh Aas]

Landed. Marking this bug fixed.

WTC - that cleanup turns out not to be necessary for this bug fix. However, I'd
still like to see it cleaned up since those macros are really confusing for
those trying to read the code. Would you rather we continue to collect approvals
on this bug or open another one? Thanks for your help!

Comment 21

[Wan-Teh Chang]

Comment on attachment 191903 [details] [diff] [review]
fix v1.0

>+#ifdef XP_MACOSX && __PowerPC__

Now sure if all compilers allow more text after the first
identifier after #ifder.  It is safer to write this as

    #if defined(XP_MACOSX) && __PowerPC__

or

    #if defined(XP_MACOSX) && defined(__PowerPC__)

Comment 22

[Simon Fraser [no longer active]]

(In reply to comment #21)
> (From update of attachment 191903 [details] [diff] [review] [edit])
> >+#ifdef XP_MACOSX && __PowerPC__
> 
> Now sure if all compilers allow more text after the first
> identifier after #ifder.  It is safer to write this as
> 
>     #if defined(XP_MACOSX) && __PowerPC__
> 
> or
> 
>     #if defined(XP_MACOSX) && defined(__PowerPC__)

It was landed as
#if defined(XP_MACOSX) && defined(__POWERPC__)

Comment 23

[Wan-Teh Chang]

Attachment: Combined patch for prlink.c (cvs diff -w)

This combined patch contains all the changes for prlink.c
proposed so far, except Simon's suggestion of including
<Carbon/Carbon.h> instead of the individual headers.

I restored the SYM_OFFSET macro and added a comment to
explain that the offset is added to skip the leading
underscore.  On Darwin, we always add a leading underscore
to the symbol's name before calling the static function
pr_FindSymbolInLib.

Comment 24

[Wan-Teh Chang]

Comment on attachment 191760 [details] [diff] [review]
Patch for prlink.h to reflect removed code

Simon: I want an accurate comment, but a non-Mac developer
won't know what "CFM" is.  Even I have forgotten what CFM is.
Do you have any other suggestion?

Comment 25

[Simon Fraser [no longer active]]

Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

>Index: prlink.c
>===================================================================

> PR_IMPLEMENT(PRLibrary*)
> PR_LoadLibraryWithFlags(PRLibSpec libSpec, PRIntn flags)
> {
>     if (flags == 0) {
>         flags = _PR_DEFAULT_LD_FLAGS;
>     }
>     switch (libSpec.type) {
>         case PR_LibSpec_Pathname:
>             return pr_LoadLibraryByPathname(libSpec.value.pathname, flags);
>-#ifdef XP_MAC
>-        case PR_LibSpec_MacNamedFragment:
>-            return pr_Mac_LoadNamedFragment(
>-                libSpec.value.mac_named_fragment.fsspec,
>-                libSpec.value.mac_named_fragment.name);
>-        case PR_LibSpec_MacIndexedFragment:
>-            return pr_Mac_LoadIndexedFragment(
>-                libSpec.value.mac_indexed_fragment.fsspec,
>-                libSpec.value.mac_indexed_fragment.index);
>-#endif /* XP_MAC */

In theory these still apply for CFM ("Code Fragment Manager") libraries on Mac
OS X, but they're probably never been used.


> static void* TV2FP(CFMutableDictionaryRef dict, const char* name, void *tvp)
> {

We dont' want any of this TVector-wrapping stuff for i386, so it should be
#ifdef __POWERPC_.

> static PRStatus
> pr_LoadViaCFM(const char *name, PRLibrary *lm)

Should pr_LoadViaCFM be #ifdeffed for powerpc only?


>     static const macLibraryLoadProc loadProcs[] = {
>-#if defined(XP_MACOSX)
>         pr_LoadViaDyld, pr_LoadCFBundle, pr_LoadViaCFM
>-#elif TARGET_CARBON
>-        pr_LoadViaCFM, pr_LoadCFBundle
>-#else
>-        pr_LoadViaCFM
>-#endif

Take out pr_LoadViaCFM for intel?


>-#if defined(XP_MAC) || defined(XP_MACOSX)
>-#if defined(NEED_LEADING_UNDERSCORE)
>+#ifdef XP_MACOSX
>+/* add this offset to skip the leading underscore in name */
> #define SYM_OFFSET 1
>-#else
>-#define SYM_OFFSET 0
>-#endif
>-#if TARGET_CARBON
>     if (lm->bundle) {
>         CFStringRef nameRef = CFStringCreateWithCString(NULL, name + SYM_OFFSET, kCFStringEncodingASCII);
>         if (nameRef) {
>             f = CFBundleGetFunctionPointerForName(lm->bundle, nameRef);
>             CFRelease(nameRef);
>         }
>     }
>-#endif
>     if (lm->connection) {
>         Ptr                 symAddr;
>         CFragSymbolClass    symClass;
>         Str255              pName;
>         
>         PR_LOG(_pr_linker_lm, PR_LOG_MIN, ("Looking up symbol: %s", name + SYM_OFFSET));
>         
>-        PStrFromCStr(name + SYM_OFFSET, pName);
>+        c2pstrcpy(pName, name + SYM_OFFSET);

As far as I can see, the function doesn't check for empty "name", so this might
copy random memory.

Comment 26

[Wan-Teh Chang]

Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

Simon:

Thanks for the code review.  I should explain the purpose
of this patch.	This patch is strictly to remove obsolete
code for the "XP_MAC" platform.  It is constructed with
the following assumptions:

1. The macro XP_MAC is never defined.
2. Whenever the macro XP_MACOSX is defined, the macro
   TARGET_CARBON is defined with the value 1.
3. Whenever the macro XP_MACOSX is defined, the macro
   NEED_LEADING_UNDERSCORE is also defined.  (This is
   true because we define NEED_LEADING_UNDERSCORE if
   DARWIN is defined.)

After the mechanical changes were made, I applied some
finishing touches.  I didn't try to fix any pre-existing
problems.

So, to save everyone's time, perhaps I should just ask
you to validate the three assumptions above, and then
ask you to review a smaller patch with just my "finishing
touches".  I don't want to waste your time in doublechecking
the mechanical changes.

Comment 27

[Josh Aas]

Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

r+ in the sense that I agree with your assumptions. I did not actually review
mechanical changes. Thanks for doing this!

Comment 28

[Simon Fraser [no longer active]]

Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

The patch is fine as "cleanup".

Comment 29

[Wan-Teh Chang]

Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

Requesting drivers' approval.

This is a code cleanup patch to eliminate dead code and
help future maintenance.  The dead code is removed by
searching for macros that are either never defined or
always defined.  After these mechanical changes are made,
I make a couple of "finishing touches" to remove an unused
variable and adjust some comments.  The risk of this patch
is extremely low.