1725587 - SpiderMonkey `IONFLAGS=codegen` printing: hierarchical created-by markers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Julian Seward [:jseward]]

(This is a spinoff from bug 1709853, without which the work there would have
been nearly impossible.)

When looking at output of IONFLAGS=codegen, it is often difficult to figure
out which of SM's numerous code generators and stub generators created a
particular instruction, and even harder to figure out which C++ routine
created it.

This patch adds a mechanism to fix that. It has zero space and time overhead
in non-debug builds by being #ifdef JS_JITSPEW, and is effective, easy to
use and to incrementally extend.

The idea is simple: class MacroAssembler has a new field, which is a vector
of const char*. This vector is operated as a stack of created-by strings,
so that

1. when we try to emit any instruction, and the stack is empty, we assert
   (in debug builds). This guarantees that all instructions can be traced
   back to at least some top-level creation function.

2. when an entry is pushed on the stack, and IONFLAGS=codegen, the entire
   stack is printed, like this (where there are 2 strings on the stack):

   [Codegen] # BEGIN creators: JitRuntime::generateBailoutTailStub/startTrampolineCode
   

   Similarly, when a value is popped off the stack, the pre-pop stack is also
   printed. This makes it possible to find the "creation stack" for any
   instruction just by looking either up or down in the debug output, for the
   closest marker.

To make it easy to use, an RAII class is provided to manage the stack. To
push a marker in a C++ scope and have it auto-removed at the end of a scope,
just do

   AutoCreatedBy acb(masm, "whatever text you like");

If any section of log output contains insufficient marker detail, just adding
a few AutoCreatedBy calls in the right places easily fixes it.

Comment 1

[Steven DeTar [:sdetar]]

After reviewing with Julian, we made this a P2

Comment 2

[Julian Seward [:jseward]]

Attachment: Bug 1725587 - SpiderMonkey `IONFLAGS=codegen` printing: hierarchical created-by markers. r=nbp.

This is an #ifdef DEBUG only patch and has no space or time effect on
non-debug builds. It makes it easier to figure out which part of the SM suite
of code generators, stub generators, etc, created each instruction shown in
the IONFLAGS=codegen output.

In the base class of all assemblers, js::jit::AssemblerShared, there is a
new field Vector<const char*> creators_. This is a small stack of arbitrary
strings. Every time a string is pushed onto or popped off the stack, the
stack is printed in the IONFLAGS=codegen output. Crucially, it is
debug-asserted that the stack is non-empty at emission of (almost) every
instruction. Hence each instruction in the log output is bracketed within at
least one level of text-label. By suitable choice of the labels, it is
possible to find which part of SM created an instruction with little
difficulty.

Pushing and popping the stack directly is strongly discouraged. Instead, an
RAII class AutoCreatedBy is provided to manage the stack. Adding
C++-scope-level annotations is then simply a matter of adding lines of the
form AutoCreatedBy acb(masm, "my tag text");.

The stacks resulting from the current annotation set have at most three
entries, for example:

BaselineInterpreterGenerator::generate/interpreter loop/op=Uint24

The patch looks large but is really very simple. There are the following
logical components, unfortunately interleaved in the patch:

• A large set of AutoCreatedBy annotations. This gives complete coverage
  for code generation on x86, x64, arm, arm64 and mips64, as far as I can
  tell. This is the vast majority of the patch.

• In class AssemblerShared (Assembler-shared.h), new field creators_ and
  methods {push,pop,has}Creator to operate on it.

• Also in Assembler-shared.h, class MOZ_RAII AutoCreatedBy, to manage the
  stack.

• The assertions to ensure that the stack is not empty at emission of any
  instruction.

  • for arm64, this is done in MozBaseAssembler::Emit (two functions)

  • for arm, this is done in Assembler::writeInst

  • for mips64, this is done in AssemblerMIPSShared::writeInst

  • for x86 and x64 I failed to find any single place to put such an
    assertion, that also has convenient access to the required
    AssemblerShared base class. Instead these have a best-effort solution:
    assertions have been added to around 20 methods in AssemblerX86Shared.
    These cover the most common instructions (loads, stores, reg-reg moves,
    pushes, pops, returns, immediate data, alignment directives). Although
    this is not complete coverage, in practice it's good enough because it's
    almost impossible to create any piece of code without using at least one
    insn in that group.

Almost all of the implementation is guarded #ifdef DEBUG. The one exception
is constructor AutoCreatedBy::AutoCreatedBy. I did not want to uglify
dozens of places in SM with guards at all its use points. So instead a dummy
release-build version has been provided:

inline AutoCreatedBy(AssemblerShared& ash, const char* who) {}

on the basis that an optimising compiler will inline it away completely.

AssemblerShared::pushCreator ignores OOM conditions when pushing on the
stack. This is assumed to be OK since this is a debug-build-only activity.
Is that OK?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c11755a1c4cf
SpiderMonkey `IONFLAGS=codegen` printing: hierarchical created-by markers.  r=nbp.

Comment 4

[Andreea Pavel [:apavel]]

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

Comment 5

[Alexandru Michis [:malexandru]]

Backed out changeset c11755a1c4cf (Bug 1725587) for causing sm bustages in BaselineCodeGen.cpp
Backout link: https://hg.mozilla.org/integration/autoland/rev/453c542ad2ef7b324eee938aa711d57eb350ccb0
Push with failures, failure log.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c3519405b7b
SpiderMonkey `IONFLAGS=codegen` printing: hierarchical created-by markers.  r=nbp.

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/3c3519405b7b

Comment 8

[Alexandru Michis [:malexandru]]

Backout from Comment 5 merged to central: https://hg.mozilla.org/mozilla-central/rev/453c542ad2ef7b324eee938aa711d57eb350ccb0

Comment 9

[Julian Seward [:jseward]]

(In reply to Alexandru Michis [:malexandru] from comment #8)

Backout from Comment 5 merged to central: https://hg.mozilla.org/mozilla-central/rev/453c542ad2ef7b324eee938aa711d57eb350ccb0

This is true [no doubt], but it's confusing in that -- per comment 7 -- the patch was
subsequently relanded and didn't bounce. It is in central now. Closing therefore.