Remove |Writer| parameter from CodeAddressService

RESOLVED FIXED in mozilla35



3 years ago
3 years ago


(Reporter: njn, Assigned: njn)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment)



3 years ago
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

3 years ago
Created attachment 8482477 [details] [diff] [review]
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.
Attachment #8482477 - Flags: review?(continuation)


3 years ago
Blocks: 1044709, 1053379
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
  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 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);
Attachment #8482477 - Flags: review?(continuation) → review+

Comment 3

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

Comment 4

3 years ago
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.