1061385 - Remove |Writer| parameter from CodeAddressService

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

CodeAddressService only allows stacks to be printed in a very particular form
(i.e. with four spaces at the start and a newline at the end). This is a
problem for making DMD's output into JSON, because I need to put the stack
entries into JSON strings.

The easy fix is to change WriteLocation() to just fill in a char buffer instead
of doing the actual writing.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove |Writer| parameter from CodeAddressService

The motivation for this was the improved flexibility in the actual printing of
the stack entry, but it's nice that it happened to simplify things as well.

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8482477 [details] [diff] [review]
Remove |Writer| parameter from CodeAddressService

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

Thanks for fixing this up.  Something did feel off here, but I couldn't put it into concrete terms.

::: xpcom/base/CodeAddressService.h
@@ +131,5 @@
>      : mEntries(), mNumCacheHits(0), mNumCacheMisses(0)
>    {
>    }
>  
> +  // Returns |aBuf|.

Is returning aBuf really that useful?  As far as I can tell, all it does is let you do
  printf("%s", GetLocation(buf));
instead of
  GetLocation(buf);
  printf("%s", buf);
That hardly seems worth behavior that's odd enough to merit a comment.  The latter also seems to make the side effects of the call less obvious.  But if you really want to save a line here, I guess it is okay.

@@ +169,5 @@
>      uintptr_t entryPc = (uintptr_t)(entry.mPc);
>      // Sometimes we get nothing useful.  Just print "???" for the entire entry
>      // so that fix-linux-stack.pl doesn't complain about an empty filename.
>      if (!entry.mFunction && !entry.mLibrary[0] && entry.mLOffset == 0) {
> +      snprintf(aBuf, aBufLen, "??? 0x%" PRIxPTR, entryPc);

Interesting, I hadn't come across PRIxPTR before. Do you have a handy reference to what it does?  Some Googling didn't turn up anything.  I'll just trust you are doing the right thing here. ;)

::: xpcom/base/nsTraceRefcnt.cpp
@@ +947,5 @@
>  static void
>  PrintStackFrameCached(void* aPC, void* aSP, void* aClosure)
>  {
> +  auto stream = static_cast<FILE*>(aClosure);
> +  static const size_t buflen = 1024;

nit: bufLen

::: xpcom/glue/BlockingResourceBase.cpp
@@ +187,5 @@
>    for (uint32_t i = 0; i < state.Length(); i++) {
> +    const size_t kMaxLength = 4096;
> +    char buffer[kMaxLength];
> +    addressService.GetLocation(state[i], buffer, kMaxLength);
> +    aOut += "    ";

The three aOut += lines should be
  aOut += nsPrintfCString("    %s\n", buffer);

Comment 3

[Nicholas Nethercote [inactive]]

Everything I know about PRIxPTR I learned from /usr/include/inttypes.h.

Comment 4

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96dd3b28ce02

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96dd3b28ce02