Implement Cell::dump and clean up dumpers a bit

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Ok, I got a little carried away when looking at bug 1277174. I was just curious what was being added to the nursery, which meant I wanted a Cell::dump(), which meant that I noticed that some thing print to a given FILE*, most print to stderr, and a few print to stdout.
Make various dumpers a little more uniform in API. Also const-ified some JSObject accessors. And use it to dump out one thing that is preventing a nursery from being empty.
Attachment #8759373 - Flags: review?(terrence)
Oops. Uh, that last patch kinda eliminated the "assert" part. It just dumped out the first Cell and continued merrily along.

This loses the immediate source location. Perhaps this would be better to return a bool, and we could continue doing

  MOZ_ASSERT(rt->gc.nursery.assertIsEmpty());

and getting the source location. As-is, this will give a "false" assertion and you'll have to look at the stack. :( But now I'm hung up on naming. checkIsEmpty(), maybe?
Attachment #8759375 - Flags: review?(terrence)
Attachment #8759373 - Attachment is obsolete: true
Attachment #8759373 - Flags: review?(terrence)
Yeah, I like checkIsEmpty() better.
Attachment #8759381 - Flags: review?(terrence)
Attachment #8759375 - Attachment is obsolete: true
Attachment #8759375 - Flags: review?(terrence)
Something else you could do would be to make nursery allocation assert when AutoAssertEmptyNursery is in place.
Oh. Duh. You are right. That would be much smarter.

So I will punish you by requesting review on the patch that adds Cell::dump. (I removed the place where AutoAssertEmptyNursery calls it; this is now just a cleanup patch.)
Attachment #8759790 - Flags: review?(jcoppeard)
Attachment #8759381 - Attachment is obsolete: true
Attachment #8759381 - Flags: review?(terrence)
Summary: Dump first cell in nursery when empty nursery assertion fails → Implement Cell::dump and clean up dumpers a bit
Comment on attachment 8759790 [details] [diff] [review]
Implement Cell::dump, and make (mostly) all dumpers accept a FILE*

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

The patch looks fine but I'm cancelling review because of the questions below.

::: js/src/jsfriendapi.h
@@ +211,5 @@
>  /*
>   * Routines to print out values during debugging.  These are FRIEND_API to help
>   * the debugger find them and to support temporarily hacking js::Dump* calls
>   * into other code.
>   */

For the functions intended to be called from the debugger it's less convenient if you have to supply the FILE* parameter.  The debugger won't supply the default it seems, and lldb can't find stderr at all.

Do we really need this parameter?  It seems like you would only use this with stderr but there are probably other cases.

I do think we should have consistency though and make all the dump methods either take it or not.

::: js/src/jsopcode.cpp
@@ -718,2 @@
>  {
>      gc::AutoSuppressGC suppressGC(cx);

Is it possible to remove these supressions, like you did for shape?

Since this is for debugging maybe we could get the runtime from TLS like GetJSContextFromJitCode does, although it looks like there is only runtimeFromMainThread() on the PerThreadData.

::: js/src/jspropertytree.cpp
@@ +376,5 @@
>      }
>  }
>  
>  void
> +Shape::dump(FILE* fp) const

This is definitely better without requiring a context.
Attachment #8759790 - Flags: review?(jcoppeard)
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07ebb2e13ef
Implement Cell::dump, and make (mostly) all dumpers accept a FILE*, r=jonco
https://hg.mozilla.org/mozilla-central/rev/f07ebb2e13ef
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Doh! Sorry, I got mixed up and thought you had r+'ed this with comments. Mind giving me a retroactive review? If the changes are simple, I'll land a followup. Otherwise, I'll back out and do it right.

MozReview-Commit-ID: 2tVz1Upt9lB
Attachment #8761272 - Flags: review?(jcoppeard)
Attachment #8759790 - Attachment is obsolete: true
Comment on attachment 8761272 [details] [diff] [review]
Implement Cell::dump, and make (mostly) all dumpers accept a FILE*,

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

As discussed on IRC, r+ but a followup simplification would be nice if it turns out we don't need the FILE* parameter.
Attachment #8761272 - Flags: review?(jcoppeard) → review+
Blocks: 1278949
You need to log in before you can comment on or make changes to this bug.