Instrument gdb to call iongraph during optimization phases.

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine: JIT
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This bug is about adding support for calling iongraph within gdb.  I can see 2 major use cases.

 1/ Being unable to reproduce a bug while logging the output of every functions.  In which case we can first use rr to reproduce the issue, and then use gdb to display the different stages of the issue once we manage to isolate it.

 2/ Being able to render in a graphical view (with color, edges, …) the content of the graph while running the debugger.

In addition, any information that we need can just be added in iongraph, as well as the current implementation of the JSONSpewer.
(Assignee)

Comment 1

2 years ago
Created attachment 8764361 [details] [diff] [review]
Add gdb commands for calling iongraph with an instance of a MIRGenerator.

This patch add multiple parameters and one commad to gdb.
First you have to setup gdb, such that it knows the default you want to use.

(gdb) set iongraph-bin /tmp/iongraph/iongraph
(gdb) set dot-bin /bin.dot
(gdb) set pngviewer-bin /usr/bin/feh

Then when you are debugging, you can use the "iongraph" command followed by
an expression which evaluates into a MIRGenerator, such as:

// Undef CodeGenerator::generateBody
(gdb) iongraph this->gen

// Under various optimization phases
(gdb) iongraph mir

And this would call iongraph, dot, and your image viewer to display the MIR
graph of the compilation, where you are stopped at the moment.
Attachment #8764361 - Flags: review?(sstangl)
(Assignee)

Comment 2

2 years ago
Created attachment 8764363 [details] [review]
Instrument iongraph to work within gdb.
Attachment #8764363 - Flags: review?(sstangl)
Comment on attachment 8764361 [details] [diff] [review]
Add gdb commands for calling iongraph with an instance of a MIRGenerator.

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

Seems OK, just some minor comments. I'm not too familiar with GDB python, but what's there looks reasonable. I'd like to see the finished version before r+.

Before landing, it looks like we'd want to get iongraph in-tree. Otherwise this won't work for too many people.

::: js/src/gdb/mozilla/IonGraph.py
@@ +52,5 @@
> +            return ""
> +        res = ""
> +        while True:
> +            if next == tail:
> +                break

In other words, `while next != tail:`

@@ +70,5 @@
> +    def get_show_string(self, value):
> +        return "Path to iongraph binary set to: %s" % value
> +    def __init__(self):
> +        super (IonGraphBinParameter, self).__init__ ("iongraph-bin", gdb.COMMAND_SUPPORT, gdb.PARAM_FILENAME)
> +        self.value = os.getenv("HOME", "~") + "/mozilla/iongraph/iongraph"

That's not where I keep it. Probably everyone keeps it in a different location, and not in $PATH. Maybe it's time to move iongraph in-tree.

@@ +92,5 @@
> +    def get_show_string(self):
> +        return "Path to a png viewer binary set to: %s" % value
> +    def __init__(self):
> +        super (PngViewerBinParameter, self).__init__ ("pngviewer-bin", gdb.COMMAND_SUPPORT, gdb.PARAM_FILENAME)
> +        self.value = "/usr/bin/feh"

It would be better to use the user's default image viewer instead of hardcoding feh. This can be done by executing 'xdg-open' instead. xdg-open is supported across FreeDesktop distros. You'll probably have to save the dot output into a temp file.

@@ +117,5 @@
> +        # From the MIRGenerator, find the graph spewer which contains both the
> +        # jsonPrinter (containing the result of the output), and the jsonSpewer
> +        # (continaining methods for spewing the graph).
> +        mirGen = gdb.parse_and_eval(mirGenExpr)
> +        jsonPrinter = mirGen['gs_']['jsonPrinter_']

What is a jsonPrinter?

@@ +143,5 @@
> +        # the state of the logging data. As this might not be the first time we
> +        # call beginFunction, we might add an extra comma at the beginning of
> +        # the output, just strip it.
> +        if json[0] == ",":
> +            json = json[1:]

This is pretty gross. Can we just add a function to jsonSpewer() that resets the logging state, and call that before beginFunction()?
Attachment #8764361 - Flags: review?(sstangl)
Comment on attachment 8764363 [details] [review]
Instrument iongraph to work within gdb.

Comments on github.
Attachment #8764363 - Flags: review?(sstangl)
(Assignee)

Comment 5

a year ago
(In reply to Sean Stangl [:sstangl] from comment #3)
> @@ +70,5 @@
> > +    def get_show_string(self, value):
> > +        return "Path to iongraph binary set to: %s" % value
> > +    def __init__(self):
> > +        super (IonGraphBinParameter, self).__init__ ("iongraph-bin", gdb.COMMAND_SUPPORT, gdb.PARAM_FILENAME)
> > +        self.value = os.getenv("HOME", "~") + "/mozilla/iongraph/iongraph"
> 
> That's not where I keep it. Probably everyone keeps it in a different
> location, and not in $PATH. Maybe it's time to move iongraph in-tree.

This is the reason, why these are parameters, such that one can change these default values within gdb, before using iongraph:

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> (gdb) set iongraph-bin /tmp/iongraph/iongraph

I thought of moving iongraph in the tree, but I think it is better out-side of mozilla-central.  As this is only a tool dedicated to Ion/Wasm developers.

I would have made a gdb instrumentation as part of iongraph, iff we did not had so many gdb instrumentation in-tree already.

(In reply to Sean Stangl [:sstangl] from comment #3)
> You'll probably have to save the
> dot output into a temp file.

Most of the tools already recognize "-" as stdout.  So, this should not be needed.

> @@ +117,5 @@
> > +        # From the MIRGenerator, find the graph spewer which contains both the
> > +        # jsonPrinter (containing the result of the output), and the jsonSpewer
> > +        # (continaining methods for spewing the graph).
> > +        mirGen = gdb.parse_and_eval(mirGenExpr)
> > +        jsonPrinter = mirGen['gs_']['jsonPrinter_']
> 
> What is a jsonPrinter?

jsonPrinter_ is a LSprinter instance, which uses the LifoAlloc of the compilation as a storage backend.  LSprinter is a chain of strings. backed by a LifoAlloc.  The json spewer instance is initialized with this json printer as the storage area.

We cannot replace this LSprinter by another, because rr refuses that we make divergent executions by settings values by hand, which is what python scripts are equivalent to.

> @@ +143,5 @@
> > +        # the state of the logging data. As this might not be the first time we
> > +        # call beginFunction, we might add an extra comma at the beginning of
> > +        # the output, just strip it.
> > +        if json[0] == ",":
> > +            json = json[1:]
> 
> This is pretty gross. Can we just add a function to jsonSpewer() that resets
> the logging state, and call that before beginFunction()?

Unfortunately no, because the compiler might see these functions as dead code, as there is no caller.  So these functions might not even exist in the final binary.  Also, even if this might be gross this works quite well, and I expect python to have a good implementation for slices.
(Assignee)

Updated

a year ago
Depends on: 1287086
(Assignee)

Comment 6

a year ago
Created attachment 8771454 [details] [diff] [review]
Add gdb commands for calling iongraph with an instance of a MIRGenerator.
Attachment #8771454 - Flags: review?(sstangl)
(Assignee)

Updated

a year ago
Attachment #8764361 - Attachment is obsolete: true
Attachment #8771454 - Flags: review?(sstangl) → review+

Comment 7

a year ago
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f1c31cfcc4
Add gdb commands for calling iongraph with an instance of a MIRGenerator. r=sstangl

Updated

a year ago
Priority: -- → P3

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6f1c31cfcc4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.