1130142 - Refcount logging should not truncate pointers

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

For some reason, refcount logging truncates pointers to 32-bit ints, which is bad.  It would also be nice if it printed them out like pointers are printed out otherwise (with lowercase letters on Linux) but if that requires touching the perl scripts the process refcount logs maybe I'm okay with leaving that alone.

Comment 1

[Andrew McCreight [:mccr8]]

Apparently this was made briefly 64-bit safe back in 2001, but then that was changed so as not to break the refcount logging scripts:
  https://bugzilla.mozilla.org/show_bug.cgi?id=20860#c58
This may result in me rewriting the refcount logging scripts in Python.

Comment 2

[Andrew McCreight [:mccr8]]

I guess this hasn't been an issue before because usually you are looking for something that leaked through shutdown, so you can figure out which object is involved entirely from the log.  I'm trying to figure out what the last two releases are for a particular object, so I need to use some external source to figure out what is the pointer I'm interested in.

Comment 3

[Andrew McCreight [:mccr8]]

Most of the perl scripts used for refcount logging are agnostic to the particular format of the address, but not surprisingly make-tree.pl does seem to depend on the format a bit, so I'll have to dig into that.

Comment 4

[Andrew McCreight [:mccr8]]

Never mind, that only matches the addresses in the stacks, not on the objects that we're logging.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Don't truncate pointers to 32-bit values in refcount logging.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47d5b8dd6469

I fixed a few other similar things in the same file.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Don't explicitly coerce pointer to bool in nsTraceRefCnt.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Use HashNumber method in more places in trace refcounting.

This should not change any behavior.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Use reinterpret_cast for PR_SetThreadPrivate.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8566936 [details] [diff] [review]
part 1 - Don't truncate pointers to 32-bit values in refcount logging.

Review of attachment 8566936 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming we can get the comment below worked out.  If you've tried it this way on Windows and it works, then great!

::: xpcom/base/nsTraceRefcnt.cpp
@@ +1103,5 @@
>      }
>  
>      bool loggingThisObject = (!gObjectsToLog || LogThisObj(serialno));
>      if (aRefcnt == 1 && gAllocLog && loggingThisType && loggingThisObject) {
> +      fprintf(gAllocLog, "\n<%s> %p %" PRIdPTR " Create\n", aClass, aPtr, serialno);

https://msdn.microsoft.com/en-us/library/vstudio/hf4y5e3w.aspx and similar suggest that Windows doesn't support the %p format specifier; it wants it spelled %P.

Might be simpler to just print everything as a hexadecimal uintptr_t...

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8566937 [details] [diff] [review]
part 2 - Don't explicitly coerce pointer to bool in nsTraceRefCnt.

Review of attachment 8566937 [details] [diff] [review]:
-----------------------------------------------------------------

I forgot to mention this on part 1, but ++ for getting rid of NS_PTR_TO_* macros.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #9)
> https://msdn.microsoft.com/en-us/library/vstudio/hf4y5e3w.aspx and similar
> suggest that Windows doesn't support the %p format specifier; it wants it
> spelled %P.

We've been using %p in for cycle collecting logging since 2010, and it works just fine on Windows.  See  NoteRefCountedObject.  The pointer output is slightly different, but that's not a big deal for refcount logging.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #9)
> > https://msdn.microsoft.com/en-us/library/vstudio/hf4y5e3w.aspx and similar
> > suggest that Windows doesn't support the %p format specifier; it wants it
> > spelled %P.
> 
> We've been using %p in for cycle collecting logging since 2010, and it works
> just fine on Windows.  See  NoteRefCountedObject.  The pointer output is
> slightly different, but that's not a big deal for refcount logging.

OK, then.  Hooray for documentation.

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #12)
> OK, then.  Hooray for documentation.

In the page I got when I clicked on the link, I see:
"p Pointer type Displays the argument as an address in hexadecimal digits."

I don't see anything about capital P.  I'm not sure what is going on there.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #13)
> I don't see anything about capital P.  I'm not sure what is going on there.

Hooray for reviewers that can't tell the difference between p and P, then. :D

Comment 15

[Andrew McCreight [:mccr8]]

Thanks for the reviews.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a6640044a109
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3c62cf81479e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/200f19f411dc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b3151e57fca

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a6640044a109
https://hg.mozilla.org/mozilla-central/rev/3c62cf81479e
https://hg.mozilla.org/mozilla-central/rev/200f19f411dc
https://hg.mozilla.org/mozilla-central/rev/5b3151e57fca