625494 - The default case in a (table)switch doesn't get traced

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b9pre) Gecko/20110112 Firefox/4.0b9pre
Build Identifier: 

Normally when a specific case or a default case in a (table)switch gets run multiple times it should get traced. In a normal case this happening. But the default case will never start to trace.

I will add a testcase. It needs to run with -j,
The testcase just runs a tableswitch and looks if tracing is enabled.
For every case in the switch it will print the case and if it is traced.

expected result:
(you can trigger this, by uncommenting the third row in the testcase. Then a lookuptable is used instead of a tableswitch)

case 0:  false
case 1:  false
default: false
case 0:  false
case 1:  false
default: false
case 0:  false
case 1:  false
default: false
case 0:  false
case 1:  false
default: true
case 0:  false
case 1:  true
default: true
case 0:  false
case 1:  true
default: true
case 0:  true
case 1:  true
default: true
case 0:  true
case 1:  true
default: true
case 0:  true


actual result:
case 0:  false
case 1:  false
default: false
case 0:  false
case 1:  false
default: false
case 0:  false
case 1:  false
default: false
case 0:  false
case 1:  false
default: false
case 0:  false
case 1:  true
default: false
case 0:  true
case 1:  true
default: false
case 0:  true
case 1:  true
default: false
case 0:  true
case 1:  true
default: false
case 0:  true



Reproducible: Always

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Testcase

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Real testcase

Sorry, in the previous testcase the third line was already uncommented. This testcase should be the right one.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Patch

I did this more as a study for myself as introduction into tracing. I'm happy that I could make a patch of it, but it isn't by any means perfect. (note: I'm only playing a week with tracing code).

So this patch traces the default case and also the normal cases. So that was the objective of this patch.
Now some important notes:
- as far as I could understand from code + comments: The original idea was to stop the tracer after the switch and start a new recording. Now I have no idea why this was done. Also for simplicity I removed that part. So tracing goes further after the switch. (If somebody could give me some explanation what the removed code actually does (ref. to the AnyAddress) plz go ahead. I can add it again if I understand what happens there.
- also there is another part I don't fully get. It is also related to the stopping of recording. I didn't removed that because it just works with that. But if somebody could give an explanation what it does and how. Plz go ahead. Also if you think it should get removed just let me know.
    jsbytecode* pc = cx->regs->pc;
    /* Starting a new trace after exiting a trace via switch. */
    if (anchor &&
        (anchor->exitType == CASE_EXIT || anchor->exitType == DEFAULT_EXIT) &&
        fragment->ip == pc) {
        return ARECORD_CONTINUE;
    }

---

Comment 4

[Hannes Verschore [:h4writer]]

the patch has some serious faults I didn't think about.
Whenever I find some time I will try to correct them.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: patch

Updated the patch. All problems should be solved.
There is one (speed) improvement I still want/need to do. That is when the default-case get's traced and the information indicates the argument is an int, then the default case don't need a check if it is an int.

Comment 6

[Brendan Eich [:brendan]]

Cc'ing tracer super-friends.

/be

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Patch

This is the updated patch, with the improvement I said above.

Only problem I observed is the following:
Assertion failure: unexpected constantly false guard detected, at jstracer.cpp:4343

On this:
function g(n) {
  switch (n) {
    case 0:
	print("case 0")
	break;
    default:
	print("default, n="+n);
	break;
  }
}

for (let i = 0; i != 50; i++) {
  g(i%4/4);
}

And I really don't have any idea how it comes. It only occurs if the input given to the switch is a 0 that is double.

Note: all these patches also solves bug #620757

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to comment #7)
> 
> Only problem I observed is the following:
> Assertion failure: unexpected constantly false guard detected, at
> jstracer.cpp:4343

I don't have time to look at this patch closely now due to other blockers I'm working on, but the two big comments on TraceRecorder::guard() should help you understand how to get rid of that assertion.

BTW, haytjes:  you must be "h4writer" on IRC, right?

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to comment #8)
> (In reply to comment #7)
> > 
> > Only problem I observed is the following:
> > Assertion failure: unexpected constantly false guard detected, at
> > jstracer.cpp:4343
> 
> I don't have time to look at this patch closely now due to other blockers I'm
> working on, but the two big comments on TraceRecorder::guard() should help you
> understand how to get rid of that assertion.

Well I had already examined that comment. But my logic tells me it shouldn't give a problem. The L_ins isn't a constant. (Higher in the code there is a check so constants don't call that guard). Also this part of the guard is really straight forward and I copied the guard from the lookup switch. So I really think the problem isn't the patch.

Now to test it, I tried and tried the same on a lookupswitch and the same happens. So also the lookupswitch has this bug.
I created a new bug for this problem: #626398

> 
> BTW, haytjes:  you must be "h4writer" on IRC, right?

Yes I'm h4writer from IRC.

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: Patch

Updated the patch. It is complete now. 
(the problem in comment 9 still occurs, but is a bug in LIR_divd and is addressed in the patch in that bug report. So isn't addressed in this patch.)

@njn: I added you as reviewer. But maybe because you didn't write that particular code I should address somebody else as reviewer, like dmandelin?

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Patch

I forgot a check in the previous patch. The one that checks if v is a number. If v isn't a number it will always take the default case, so recording can happily continue.

Comment 12

[Hannes Verschore [:h4writer]]

Attachment: Unradical patch

This patch isn't an overhaul of the switch tracerecorder. It's only a simple patch that just adds the minimum code to add tracing to the default case.

Comment 13

[Hannes Verschore [:h4writer]]

This patch still needs review. It is a really easy patch and could introduce a lot of performance when the default-case is used.

Comment 14

[Nicholas Nethercote [inactive]]

Haytjes, sorry for the delay.  I plan to review this but we're all focussed completely on finishing Firefox 4.0, because it's 4 months late.  As soon as it's released I'll do the review.  Thanks for your patience.

Comment 15

[Nicholas Nethercote [inactive]]

Bug 620757 stopped tracing table switches, thus rendering this bug moot.