Last Comment Bug 303345 - add Universal Binary (mach-o FAT) plugin support
: add Universal Binary (mach-o FAT) plugin support
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: ---
Assigned To: Josh Aas
:
Mentors:
http://developer.apple.com/documentat...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-08-03 23:12 PDT by Josh Aas
Modified: 2005-08-12 14:50 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
prlink mac cleanup, v1.0 (33.52 KB, patch)
2005-08-04 12:48 PDT, Josh Aas
wtc: review+
Details | Diff | Review
Mark and Wan-Teh's suggested changes (cvs diff -w) (15.79 KB, patch)
2005-08-05 17:24 PDT, Wan-Teh Chang
jaas: review+
Details | Diff | Review
Patch for prlink.h to reflect removed code (1.09 KB, patch)
2005-08-05 17:41 PDT, Wan-Teh Chang
jaas: review+
sfraser_bugs: superreview+
benjamin: approval1.8b4+
Details | Diff | Review
fix v1.0 (737 bytes, patch)
2005-08-07 12:51 PDT, Josh Aas
mikepinkerton: review+
sfraser_bugs: superreview+
asa: approval1.8b4+
Details | Diff | Review
Combined patch for prlink.c (cvs diff -w) (33.98 KB, patch)
2005-08-08 13:33 PDT, Wan-Teh Chang
jaas: review+
sfraser_bugs: superreview+
asa: approval1.8b4+
Details | Diff | Review

Description Josh Aas 2005-08-03 23:12:46 PDT
We crash trying to use Universal Binary plugins on Mac OS X. We need to learn to
deal with them.
Comment 1 Josh Aas 2005-08-04 12:48:19 PDT
Created attachment 191618 [details] [diff] [review]
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 2005-08-05 11:34:51 PDT
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 2005-08-05 16:17:56 PDT
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 2005-08-05 17:00:05 PDT
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 2005-08-05 17:12:10 PDT
I will make the two changes that Mark suggested in comment 2.
Comment 6 Wan-Teh Chang 2005-08-05 17:24:11 PDT
Created attachment 191756 [details] [diff] [review]
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 2005-08-05 17:41:39 PDT
Created attachment 191760 [details] [diff] [review]
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 2005-08-05 17:42:53 PDT
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 2005-08-05 18:21:32 PDT
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 2005-08-05 18:24:46 PDT
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 2005-08-07 00:20:07 PDT
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 2005-08-07 12:51:37 PDT
Created attachment 191903 [details] [diff] [review]
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) 2005-08-07 14:14:52 PDT
Comment on attachment 191903 [details] [diff] [review]
fix v1.0

r=pink
Comment 14 Simon Fraser 2005-08-07 14:37:47 PDT
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 2005-08-07 14:56:58 PDT
Its POWERPC, or at least that has been tested and works for me. I'll fix casing
on checkin.
Comment 16 Josh Aas 2005-08-07 14:58:19 PDT
I should note though, that both __PowerPC__ and __POWERPC__ work for me, so my
patch isn't technically wrong.
Comment 17 Simon Fraser 2005-08-07 15:09:14 PDT
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 2005-08-07 16:13:27 PDT
(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 2005-08-07 16:19:50 PDT
__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 2005-08-07 21:48:25 PDT
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 2005-08-08 11:14:15 PDT
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 2005-08-08 12:20:27 PDT
(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 2005-08-08 13:33:22 PDT
Created attachment 192005 [details] [diff] [review]
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 2005-08-08 13:38:03 PDT
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 2005-08-08 13:58:01 PDT
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 2005-08-08 14:11:17 PDT
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 2005-08-08 14:14:47 PDT
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 2005-08-08 15:12:16 PDT
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 2005-08-08 15:24:27 PDT
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.

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