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

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Nick Kreeger]

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.

Comment 1

[Nick Kreeger]

Attachment: Proposed Patch v1

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.

Comment 2

[Wan-Teh Chang]

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.

Comment 3

[Nick Kreeger]

Attachment: Patch version 2.

Comment 4

[Mark Mentovai]

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.

Comment 5

[Nick Kreeger]

Attachment: Patch Version 3

Round 3... addresses all of mento's comments

Comment 6

[Mark Mentovai]

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);

Comment 7

[Nick Kreeger]

Attachment: Patch Version 4

Round 4... *really* addresses all of mento's comments.

Comment 8

[Mark Mentovai]

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.

Comment 9

[Nick Kreeger]

Attachment: Patch Version 5

This patch addresses mento's comments. Wan-teh, would you please review this. 

FYI: I patched off of the MOZILLA_1_8_BRANCH.

Comment 10

[Mark Mentovai]

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.

Comment 11

[Nick Kreeger]

Poking this bug, can I get an update status on this patch, It's hung around for a while...

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Nick, what about Mark's comment 10?

Comment 13

[Nick Kreeger]

Attachment: Patch V6

Phew... Sorry for the delay... Here is another spin based on Mento's comments.

Comment 14

[Mark Mentovai]

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.

Comment 15

[Wan-Teh Chang]

Attachment: Nick Kreeger's Patch V6.1 (as checked in)

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

Comment 16

[Wan-Teh Chang]

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.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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).

Comment 18

[Wan-Teh Chang]

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).

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.