If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Range analysis code needs actual unit tests

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Waldo, Assigned: sunfish, Mentored)

Tracking

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

Right now the range analysis code is only tested indirectly, through jit-tests, jstests, etc.  But this problem is narrowly scoped enough that we can, and should, test the range analysis code directly at the C++ level, in jsapi-tests (that use purely internal headers, but it's okay for JSAPI tests to do that).  It's a whole lot easier constructing debugging a jsapi-test of range analysis stuff, than it is of JS code that happens to invoke the range analysis code in the same way.  (Also, JS might constant-fold and do other things such that it's difficult to impossible to invoke range analysis with the precise desired inputs, which might hide bugs thought impossible.)

There's a little messiness here in that Range is TempObject right now, but I imagine throwing some C++ templates at the problem, and having (say) |template<class Base> class RangeImpl : public Base| with |typedef RangeImpl<TempObject> Range;| or something would solve things pretty nicely.
(Assignee)

Comment 1

3 years ago
js/src/jsapi-tests/testJitRValueAlloc.cpp is now an example of a JIT test in jsapi-tests.
This bug is not a good first bug, as it requires a bit of knowledge about Range Analysis.  I would recommend to have a look at the work being done in order to improve our documentation on Range Analysis (Bug 1028274)

The goal of adding unit-test for Range Analysis is first to test all functions which are made on Range [1], and ensure that if we provide any value within the input range, then the result is within the output range.

Among interesting values which are interesting in terms of boundaries of inputs / outputs ranges, we have:

  ±Inf
  ±Math.pow(2,52) (± 1)
  ±Math.pow(2,32) (± 1)
  ±Math.pow(2,31) (± 1)
  ±Math.pow(2,n) (± 0.5) [n < 32]
  ±0

So there is a lot of potential test cases to make and a lot of values to iterate over.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/RangeAnalysis.h?from=RangeAnalysis.h#398-418
Mentor: nicolas.b.pierron@mozilla.com
Whiteboard: [lang=c++]
(Assignee)

Comment 3

3 years ago
Bug 1036037 added js/src/jsapi-tests/testJitFoldsTo.cpp, which is an example of a JIT test in jsapi-tests that builds a MIRGraph.
Assignee: general → nobody
(Assignee)

Comment 4

3 years ago
Created attachment 8519105 [details] [diff] [review]
range-unit-test.patch

This test redoes the test from bug 1094052 to be more unit-test-like, calling the Range class functions directly, rather than going through all the trouble of building a MIRGraph. I think this is a good example of what we want unit tests for the Range class to look like.

It also adds a test for bug 1093356, which is a good example of a testcase which does need to build a full MIRGraph.
Assignee: nobody → sunfish
Attachment #8519105 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8519105 [details] [diff] [review]
range-unit-test.patch

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

::: js/src/jsapi-tests/testJitRangeAnalysis.cpp
@@ +52,5 @@
> +    Range *zero = Range::NewDoubleSingletonRange(func.alloc, 0.0);
> +    Range *half = Range::NewDoubleSingletonRange(func.alloc, 0.5);
> +    Range *one = Range::NewDoubleSingletonRange(func.alloc, 1.0);
> +    Range *threehalves = Range::NewDoubleSingletonRange(func.alloc, 1.5);
> +    Range *inf = Range::NewDoubleSingletonRange(func.alloc, mozilla::PositiveInfinity<double>());

nit: I think that using variables with identical length would improve the readability of this test:

  _nan
  ninf
  n1_5
  n1_0
  n0_5
  n0_0
  p0_0
  p0_5
  p1_0
  p1_5
  pinf
Attachment #8519105 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 6

3 years ago
Done. https://hg.mozilla.org/integration/mozilla-inbound/rev/b27100f83983
https://hg.mozilla.org/mozilla-central/rev/b27100f83983
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.