1277690 - Implement Cell::dump and clean up dumpers a bit

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

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.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Dump first cell in nursery when empty nursery assertion fails

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.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Dump first cell in nursery when empty nursery assertion fails

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?

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Dump first cell in nursery when empty nursery assertion fails

Yeah, I like checkIsEmpty() better.

Comment 4

[Jon Coppeard (:jonco)]

Something else you could do would be to make nursery allocation assert when AutoAssertEmptyNursery is in place.

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Implement Cell::dump, and make (mostly) all dumpers accept a FILE*

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.)

Comment 6

[Jon Coppeard (:jonco)]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07ebb2e13ef
Implement Cell::dump, and make (mostly) all dumpers accept a FILE*, r=jonco

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f07ebb2e13ef

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Implement Cell::dump, and make (mostly) all dumpers accept a FILE*,

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

Comment 10

[Jon Coppeard (:jonco)]

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.