Closed Bug 1130142 Opened 5 years ago Closed 5 years ago

Refcount logging should not truncate pointers

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.
Blocks: 1130158
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.
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.
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.
Never mind, that only matches the addresses in the stacks, not on the objects that we're logging.
This should not change any behavior.
Attachment #8566938 - Flags: review?(nfroyd)
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...
Attachment #8566936 - Flags: review?(nfroyd) → review+
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.
Attachment #8566937 - Flags: review?(nfroyd) → review+
Attachment #8566938 - Flags: review?(nfroyd) → review+
Attachment #8566940 - Flags: review?(nfroyd) → review+
(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.
(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.
(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.
(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
You need to log in before you can comment on or make changes to this bug.