Closed Bug 354248 Opened 13 years ago Closed 12 years ago

Enable NSLinkEditError logging for |pr_LoadViaDyld()| in prlink.c

Categories

(NSPR :: NSPR, defect)

PowerPC
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nick.kreeger, Assigned: nick.kreeger)

Details

Attachments

(1 file, 6 obsolete files)

In order to get some more information about why a dylib isn't getting loaded we should implement the NSLinkEditError API's in prlink.c.
Attached patch Proposed Patch v1 (obsolete) — Splinter Review
Here is the first version of the patch. I'm not sure if using a |printf| call to dump the log is the best way, but it was what I was using.
Attachment #240090 - Flags: review?(mark)
Comment on attachment 240090 [details] [diff] [review]
Proposed Patch v1

Use PR_LOG instead of printf.

It should be an error to pass uninitialized
linkEditErrors to NSLinkEditError.

Ideally, the error number and error string should
be returned to the caller so that eventually they
are passed to the PR_SetError and PR_SetErrorText
calls, but that will require much more changes.
Attached patch Patch version 2. (obsolete) — Splinter Review
Attachment #240090 - Attachment is obsolete: true
Attachment #240188 - Flags: review?(mark)
Attachment #240090 - Flags: review?(mark)
QA Contact: wtchang → nspr
Comment on attachment 240188 [details] [diff] [review]
Patch version 2.

Nick, sorry I lost track of this.

>+#ifdef DEBUG
>+      NSLinkEditErrors* linkEditErrors = NULL;
>+        int errorNum;
>+        const char* errorString;
>+        NSLinkEditError(linkEditErrors, &errorNum, &name, &errorString);

NSLinkEditError tries to put a code into linkEditErrors, which is NULL here.  This will crash.  You should do:

NSLinkEditErrors linkEditError;
NSLinkEditError(&linkEditError, &errorNum, ...)

Don't reuse (and overwrite) |name|, declare and use a new const char *.

>+        PR_LOG(_pr_linker_lm, PR_LOG_MIN, 
>+              ("LoadMachDyldModule ERROR: %s CODE: %i", errorString, errorNum));

errorString is a multi-line string.  Better to put the code first.  errorNum is not meaningful without linkEditError.  Hw about:

PR_LOG(_pr_linker_lm, PR_LOG_MIN, 
       ("LoadMachDyldModule error %i:%i for file %s:\n%s",
        linkEditError, errorNum, filename, errorString));

Also, consolidate your indentation.
Attachment #240188 - Flags: review?(mark) → review-
Attached patch Patch Version 3 (obsolete) — Splinter Review
Round 3... addresses all of mento's comments
Attachment #240188 - Attachment is obsolete: true
Attachment #253045 - Flags: review?(mark)
Comment on attachment 253045 [details] [diff] [review]
Patch Version 3

>+#ifdef DEBUG

I don't see why this needs to be #ifdef DEBUG.

>+        NSLinkEditErrors* linkEditErrors = NULL;

This is still wrong.  It should cause a warning below where you call NSLinkEditError since this is C code; if it were C++, this wouldn't even compile.  It will also cause all sorts of nasty stack corruption, quite possibly resulting in a crash.  You want linkEditErrors to be a local variable with storage allocated on the stack, you don't want it to be a *pointer.  You should take &addressof that buffer when you call NSLinkEditError, you don't want to do &addressof a *pointer, which results in a **pointer-to-pointer - not what the callee asks for.  Look at the example from comment 4 again.

[...]
>+        NSLinkEditError(&linkEditErrors, &errorNum, &fileName, &errorString);
Attachment #253045 - Flags: review?(mark) → review-
Attached patch Patch Version 4 (obsolete) — Splinter Review
Round 4... *really* addresses all of mento's comments.
Attachment #253045 - Attachment is obsolete: true
Attachment #253511 - Flags: review?(mark)
Comment on attachment 253511 [details] [diff] [review]
Patch Version 4

Rename linkEditErrors to linkEditError (since there's only one of them - the pluralization in NSLinkEditErrors refers to the enum which contains multiple values) and r=me.
Attachment #253511 - Flags: review?(mark) → review+
Attached patch Patch Version 5 (obsolete) — Splinter Review
This patch addresses mento's comments. Wan-teh, would you please review this. 

FYI: I patched off of the MOZILLA_1_8_BRANCH.
Attachment #254194 - Flags: review?(wtchang)
Comment on attachment 254194 [details] [diff] [review]
Patch Version 5

Actually, the entire new block should be wrapped in |if (!lm->image)| or |if (lm->image == NULL)|, which seems to better match the surrounding style.
Poking this bug, can I get an update status on this patch, It's hung around for a while...
Nick, what about Mark's comment 10?
Attached patch Patch V6 (obsolete) — Splinter Review
Phew... Sorry for the delay... Here is another spin based on Mento's comments.
Attachment #253511 - Attachment is obsolete: true
Attachment #254194 - Attachment is obsolete: true
Attachment #287117 - Flags: superreview?
Attachment #287117 - Flags: review?(mark)
Attachment #254194 - Flags: review?(wtc)
Attachment #287117 - Flags: superreview? → superreview?(wtc)
Comment on attachment 287117 [details] [diff] [review]
Patch V6

>Index: prlink.c

>+            PR_LOG(_pr_linker_lm, PR_LOG_MIN, 
>+                   ("LoadMachDyldModule error %i:%i for file %s:\n%s",
>+                     linkEditError, errorNum, fileName, errorString));

I'd line the l in linkEditError on the third line above up under the " instead of the L in the second line.  Looks good.
Attachment #287117 - Flags: review?(mark) → review+
r=wtc.  I made some minor formatting changes and changed
%i to %d in the PR_LOG format string.

I checked in your patch on the NSPR trunk for NSPR 4.7.

Checking in prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.93; previous revision: 3.92
done
Attachment #287117 - Attachment is obsolete: true
Attachment #289590 - Flags: review+
Attachment #287117 - Flags: superreview?(wtc)
Nick, thanks a lot for your patch.

A suggestion for future work (low priority):

Right now after pr_LoadViaDyld failure, pr_LoadLibraryByPathname
does this:

    if (status != PR_SUCCESS) {
        oserr = cfragNoLibraryErr;
        PR_DELETE(lm);
        goto unlock;
    }

and then this:

  unlock:
    if (result == NULL) {
        PR_SetError(PR_LOAD_LIBRARY_ERROR, oserr);
        DLLErrorInternal(oserr);  /* sets error text */
    }

Ideally, we should somehow cause pr_LoadLibraryByPathname to
call PR_SetErrorText to save the error string returned by
NSLinkEditError, or set 'oserr' to the errorNum returned
by NSLinkEditError.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
Could the same (similar) code have been used in pr_LoadMachDyldModule http://mxr.mozilla.org/nspr/source/nsprpub/pr/src/linking/prlink.c#489 ?

We hit a module loading error last week in Camino-on-1.9.3 that had a useless error message that Stuart thought came through the unpatched codepath (the power of the internet led him back to this bug, where Nick had patched the other codepath).
I think so.  It is best to open a new bug, reference this bug as
related, and submit a patch (since you ran into a module loading
error and therefore have the setup to test a patch).
Thanks, Wan-Teh; I filed bug 571425.  Unfortunately it looks like the follow-up work mentioned in comment 16 is needed to get the detailed error messages up to XPCOM, but implementing the logging in NSPR a start.
You need to log in before you can comment on or make changes to this bug.