Closed Bug 1369410 Opened 2 years ago Closed 2 years ago

Enable JSVM code coverage collection on linux64-ccov.


(Testing :: Code Coverage, enhancement)

Not set


(firefox55 fixed)

Tracking Status
firefox55 --- fixed


(Reporter: sparky, Assigned: sparky)


(Depends on 3 open bugs, Blocks 1 open bug)



(1 file)

Enable JSVM code coverage collection on linux64-ccov. There is a patch ready for this, but there are still jit-test errors left to fix:

1. I think 'Script-getOffsetsCoverage-02.js' should be skipped because it expects code coverage to be disabled when it starts. I imagine this test will also be a problem on linux64-jsdcov.

2. 'werror.js', and 'bug1186973.js' test javascript errors. I have a feeling that this has something to do with priorities. The test simply times out and there is no log information.

3. The rest of the tests all fail because the javascript function names are rewritten to a "nice" form before comparison to check if they are lazy. The functions which fail in the comparisons are all lazy functions. The lcov output file 'CodeCoverage.cpp' has "no effect" on this. In other words, when we disable all the collection processes (leaving us with empty .info files) the error is still there so it has nothing to do with the actual collection. So, I believe there is a hook somewhere that's causing this interference. (Something to do with delazification?)
:nbp would you have any ideas about how 2 or 3 could be fixed or where the problems are coming from?
Flags: needinfo?(nicolas.b.pierron)
I spoke with nbp over IRC and he mentioned that problem 3 is expected because we can't relazify after a lazy function is called since we need to keep the coverage count. So, that means that we can skip these tests, along with the tests mentioned in 1.

There is still some work that needs to be done to see if anything can be done for 2.
Flags: needinfo?(nicolas.b.pierron)
For 3, javascript runtime errors are handled everywhere and there does not seem to be any common area that could be used to our advantage. To me, this means that we would have to make a lot of modifications to JIT/IonMonkey to get coverage on purposely failing tests for very little gain.

Joel, I think we can skip all the jit failures. 1 is for coverage tests, which pass when coverage is not running, but fail when it is already running which is expected since the tests expect there to be no code coverage instrumentation enabled when they run. 2 is for the lazy function errors, and as nbp noted, we can't relazify them after because we would lose the coverage counts so these errors need to be skipped. And 3, as I explained above, doesn't provide us with much of a gain in comparison to what we would have to do with JIT/IonMonkey, in theory (I'm going to look into this one some more). What do you think?
Flags: needinfo?(jmaher)
I am all for skipping tests; lets make sure that we do it in an easy to query way so in the future it is straightforward to find all tests skipped, etc. :)
Flags: needinfo?(jmaher)
I am thinking of adding this comment: # skip-if = coverage for jit-test

That would make it easier to find all tests being skipped for coverage on DXR. I'll make two bugs to document the problems, one for the runtime error failures and one for the lazy function problem. (No bug for the coverage error, since that's self-explanatory). 

So I'll skip the tests, write the bugs, test the jsvm on all test suites again and then submit a patch for review.
Depends on: 1369783
Depends on: 1369785
Here's a test run with JSVM on all test-suites with the tests disabled:

I can submit the patch now or after bug 1367763 has landed. Either way, one or the other will need to be modified slightly.
lets submit for review so this can land- the jittests are all green right now!
Assignee: nobody → gmierz2
Comment on attachment 8873898 [details]
Bug 1369410 - Enable JSVM code coverage collection on linux64-ccov.

thanks, this looks great.
Attachment #8873898 - Flags: review?(jmaher) → review+
Pushed by
Enable JSVM code coverage collection on linux64-ccov. r=jmaher
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.