1235641 - Add line and column info to dump bytecode basic blocks

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sander Mathijs van Veen]

Attachment: add-line-column-info-to-dump-bytecode.patch

The attached patch adds JavaScript line and column information to Ion's dumped basic blocks. Currently, the basic blocks that are dumped using the --dump-bytecode flag only have an offset and are delimited by "--- SCRIPT ... ---" lines. Adding line and column information makes it easier to see what JavaScript code generated which basic block.

Is this related to https://bugzilla.mozilla.org/show_bug.cgi?id=1178840 ?

This is different from IonGraph / jitspew because this does not require an alternative build (enable jitspew). Every normal release build of the js shell can emit line/column info using this patch.

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8702693 [details] [diff] [review]
add-line-column-info-to-dump-bytecode.patch

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

Sadly, I do not think this would work, because the script on which we store the IonScriptCounts is the top-level script, thus

::: js/src/jsopcode.cpp
@@ +154,5 @@
>      Sprint(sp, "IonScript [%lu blocks]:\n", ionCounts->numBlocks());
>      for (size_t i = 0; i < ionCounts->numBlocks(); i++) {
>          const jit::IonBlockCounts& block = ionCounts->block(i);
> +        unsigned lineNumber = 0, columnNumber = 0;
> +        jsbytecode *offset = script->code() + block.offset();

this might lead to incorrect reports as we can have basic block inlined from other functions.

Comment 2

[Sander Mathijs van Veen]

Attachment: redundant-bitand-with-constant-call2.js.log

Consider the following test case:

     1	
     2	// These empty lines are here to test the line numbering
     3	
     4	(function() {
     5	
     6	    function g(x) {
     7	        if ((x & 1) == 1) return 1;
     8	        return 2;
     9	    }
    10	
    11	    function f(n) {
    12	        var q = 0;
    13	        for (var i = 0; i < n; i++)
    14	            q += g(i);
    15	        return q;
    16	    }
    17	
    18	    var sum = 0;
    19	    for (var i = 0; i < 20000; i++) {
    20	        sum += f(1000);
    21	    }
    22	    print(sum);
    23	    //print(f(1000));
    24	})();

There are a few blank lines added explicitly to test if the line numbering works correctly.

The log file contains the generated bytecode and assembly when the patch is applied. Note that inlined functions are marked as such:

    BB #3 [00033,14,17] [inlined redundant-bitand-with-constant-call2.js:6] -> #4 -> #5 :: 19994000 hits
    [MoveGroup]
    movl       %edx, %ebp
    [BitOpI:bitand]
    andl       $0x1, %ebp
    [CompareAndBranch:eq]
    cmpl       $0x1, %ebp
    jne        .Lfrom130

    BB #4 [00033,14,17] [inlined redundant-bitand-with-constant-call2.js:6] -> #6 :: 9997000 hits
    [Integer]
    movl       $0x1, %ebp
    [Goto]
    jmp        .Lfrom154

    BB #5 [00033,14,17] [inlined redundant-bitand-with-constant-call2.js:6] -> #6 :: 9997000 hits
    [Integer]
    movl       $0x2, %ebp
    [Goto]

Do you have an example in mind that will produce the wrong line numbering?

Comment 3

[Sean Stangl [:sstangl]]

Comment on attachment 8702693 [details] [diff] [review]
add-line-column-info-to-dump-bytecode.patch

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

(In reply to Sander Mathijs van Veen from comment #2)
> Do you have an example in mind that will produce the wrong line numbering?

Nicolas' point is that for inlined functions, they will show only the line information of the inlining site in the caller.

In the example you gave, the basic blocks of `g()` are shown as being part of line 14, their callsite. Nicolas thinks this would be confusing, because one would expect those blocks to be noted as from lines 7 and 8, their actual source.

FWIW I'm fine with this patch.

::: js/src/jsopcode.cpp
@@ +155,5 @@
>      for (size_t i = 0; i < ionCounts->numBlocks(); i++) {
>          const jit::IonBlockCounts& block = ionCounts->block(i);
> +        unsigned lineNumber = 0, columnNumber = 0;
> +        jsbytecode *offset = script->code() + block.offset();
> +        if (offset) {

This looks fishy. Why is a nullptr check required here? And why is it checking the result of an addition, instead of just script->code()?

Comment 4

[Sean Stangl [:sstangl]]

Thanks for the patch, BTW. I forget to say that sometimes!

Comment 5

[Sander Mathijs van Veen]

Attachment: bug1235641-add-line-and-column-info-to-dump-bytecode-basic-blocks.patch

This patch uses script->offsetToPC(block.offset()) instead of the if and plus operator. IMHO this seems much better than using that check (which could be incorrect).

Comment 6

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bf587a840a4

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17d2806c96d7

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17d2806c96d7

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17d2806c96d7