1397717 - Move *::dump() to GenericPrinter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

We have many implementations of dump() that use FILE*. For unit testing purposes, it can be very useful to be able to dump to a string. Fortunately, we have GenericPrinter for this, so let's make use of it.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

We have a host of DEBUG-only dump()-style APIs that output either to
stderr or to a FILE*. Unfortunately, this means that we cannot use
these dumps e.g. for unit tests. This patch ports most of the dump()
APIs to use GenericPrinter, which is more flexible.

Some parts of the code have not been ported, in particular
TypeInference, which still uses stderr.

Review commit: https://reviewboard.mozilla.org/r/177306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177306/

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

https://reviewboard.mozilla.org/r/177306/#review182324

::: js/src/frontend/ParseNode.h:822
(Diff revision 1)
>          MOZ_ASSERT(NodeType::test(*this));
>          return *static_cast<const NodeType*>(this);
>      }
>  
>  #ifdef DEBUG
> -    void dump();
> +    void dump(GenericPrinter& out);

nit: The generic printer is not gdb friendly, add a dump() function which delegate to calling the with  "Fprinter out(stderr);"

::: js/src/jsobj.cpp:3724
(Diff revision 1)
>                              i.script(), i.script()->pcToOffset(i.pc()));
>          } else {
>              sprinter.printf(" (%p)\n", i.pc());
>          }
>      }
> -    fprintf(fp, "%s", sprinter.string());
> +    out.printf("%s", sprinter.string());

open a follow-up good first bug to replace sprinter by the generic printer in this function.

::: js/src/vm/Shape.cpp:1796
(Diff revision 1)
>          MOZ_ASSERT(JSID_IS_SYMBOL(propid));
> -        JSID_TO_SYMBOL(propid)->dump(fp);
> +        JSID_TO_SYMBOL(propid)->dump(out);
>      }
>  
> -    fprintf(fp, " g/s %p/%p slot %d attrs %x ",
> +    out.printf(" g/s %p/%p slot %d attrs %x ",
>              JS_FUNC_TO_DATA_PTR(void*, getter()),

nit: fix the indentation after out.printf new-lines.

::: js/src/vm/String.h:531
(Diff revision 1)
>      }
>  
>      static const JS::TraceKind TraceKind = JS::TraceKind::String;
>  
>  #ifdef DEBUG
> -    void dump(FILE* fp);
> +    void dump(js::GenericPrinter& out);

nit: add a dump() function which defaults to stderr.

::: js/src/vm/Symbol.h:98
(Diff revision 1)
>      size_t sizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) const {
>          return mallocSizeOf(this);
>      }
>  
>  #ifdef DEBUG
> -    void dump(FILE* fp = stderr);
> +    void dump(js::GenericPrinter& out);

nit: add a dump() function, for gdb-friendly reasons.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177306/diff/1-2/

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

https://reviewboard.mozilla.org/r/177306/#review182452

::: js/src/frontend/ParseNode.h:1358
(Diff revisions 1 - 2)
> +// Debugger-friendly stderr printer.
>  void DumpParseTree(ParseNode* pn, GenericPrinter& out, int indent = 0);

How is the GenericPrinter& out debugger friendly?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177306/diff/2-3/

Comment 6

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

https://reviewboard.mozilla.org/r/177306/#review182728

::: js/src/jsobj.cpp:3695
(Diff revisions 2 - 3)
> +JS_FRIEND_API(void)
>  js::DumpBacktrace(JSContext* cx, js::GenericPrinter& out)

Maybe we should keep all GenericPrinter functions out of the JS_FRIEND_API interface?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177306/diff/3-4/

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177306/diff/4-5/

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177306/diff/5-6/

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177306/diff/6-7/

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8905491 [details]
Bug 1397717 - Using GenericPrinter for DEBUG-only C++ dump() APIs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177306/diff/7-8/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95bb61612572
Using GenericPrinter for DEBUG-only C++ dump() APIs;r=nbp

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/95bb61612572