Closed
Bug 303345
Opened 19 years ago
Closed 19 years ago
add Universal Binary (mach-o FAT) plugin support
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jaas, Assigned: jaas)
References
()
Details
Attachments
(3 files, 2 obsolete files)
1.09 KB,
patch
|
jaas
:
review+
sfraser_bugs
:
superreview+
benjamin
:
approval1.8b4+
|
Details | Diff | Splinter Review |
737 bytes,
patch
|
mikepinkerton
:
review+
sfraser_bugs
:
superreview+
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
33.98 KB,
patch
|
jaas
:
review+
sfraser_bugs
:
superreview+
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
We crash trying to use Universal Binary plugins on Mac OS X. We need to learn to deal with them.
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.
Attachment #191618 -
Flags: review?(wtchang)
Comment 2•19 years ago
|
||
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 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.
Attachment #191618 -
Attachment is obsolete: true
Attachment #191618 -
Flags: review?(wtchang)
Comment 4•19 years ago
|
||
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?
Attachment #191618 -
Flags: review+
Comment 6•19 years ago
|
||
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.
Attachment #191756 -
Flags: review?(joshmoz)
Comment 7•19 years ago
|
||
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).
Attachment #191760 -
Flags: review?(joshmoz)
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?
Attachment #191756 -
Flags: review?(joshmoz) → review+
Attachment #191760 -
Flags: review?(joshmoz)
Attachment #191760 -
Flags: review+
Attachment #191760 -
Flags: approval1.8b4?
Attachment #191756 -
Flags: superreview?(sfraser_bugs)
Attachment #191618 -
Attachment is obsolete: false
Attachment #191618 -
Flags: approval1.8b4?
Comment 9•19 years ago
|
||
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"
Attachment #191760 -
Flags: superreview+
Comment 10•19 years ago
|
||
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 ?
Updated•19 years ago
|
Attachment #191760 -
Flags: approval1.8b4? → approval1.8b4+
Updated•19 years ago
|
Attachment #191618 -
Attachment is obsolete: true
Attachment #191618 -
Flags: approval1.8b4?
Assignee | ||
Comment 11•19 years ago
|
||
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.
Flags: blocking1.8b4?
Assignee | ||
Comment 12•19 years ago
|
||
Don't use CFM function pointers on Mac OS X x86. Allows UB plugins to work on x86.
Attachment #191903 -
Flags: review?(sfraser_bugs)
Comment 13•19 years ago
|
||
Comment on attachment 191903 [details] [diff] [review] fix v1.0 r=pink
Attachment #191903 -
Flags: review?(sfraser_bugs) → review+
Attachment #191903 -
Flags: superreview?(sfraser_bugs)
Comment 14•19 years ago
|
||
Comment on attachment 191903 [details] [diff] [review] fix v1.0 So in tree I see __POWERPC__ and __powerpc__, but not __PowerPC__. Which is it?
Assignee | ||
Comment 15•19 years ago
|
||
Its POWERPC, or at least that has been tested and works for me. I'll fix casing on checkin.
Assignee | ||
Comment 16•19 years ago
|
||
I should note though, that both __PowerPC__ and __POWERPC__ work for me, so my patch isn't technically wrong.
Comment 17•19 years ago
|
||
Comment on attachment 191903 [details] [diff] [review] fix v1.0 sr with +#ifdef XP_MACOSX && __PowerPC__ changed to +#if defined(XP_MACOSX) && defined(__POWERPC__)
Attachment #191903 -
Flags: superreview?(sfraser_bugs) → superreview+
Attachment #191903 -
Flags: approval1.8b4?
Comment 18•19 years ago
|
||
(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.
Assignee | ||
Comment 19•19 years ago
|
||
__ppc__ never works for me - I've tried it at your request a few times already, so we can't do that
Updated•19 years ago
|
Attachment #191903 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 20•19 years ago
|
||
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!
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 21•19 years ago
|
||
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•19 years ago
|
||
(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__)
Updated•19 years ago
|
Flags: blocking1.8b4?
Comment 23•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #191756 -
Attachment is obsolete: true
Attachment #192005 -
Flags: superreview?(sfraser_bugs)
Attachment #192005 -
Flags: review?(joshmoz)
Comment 24•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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.
Assignee | ||
Comment 27•19 years ago
|
||
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!
Attachment #192005 -
Flags: review?(joshmoz) → review+
Comment 28•19 years ago
|
||
Comment on attachment 192005 [details] [diff] [review] Combined patch for prlink.c (cvs diff -w) The patch is fine as "cleanup".
Attachment #192005 -
Flags: superreview?(sfraser_bugs) → superreview+
Updated•19 years ago
|
Attachment #191756 -
Flags: superreview?(sfraser_bugs)
Comment 29•19 years ago
|
||
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.
Attachment #192005 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #192005 -
Flags: approval1.8b4? → approval1.8b4+
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•